Skip to content
This repository has been archived by the owner on Jan 17, 2018. It is now read-only.

Refactored python script #13

Merged
merged 13 commits into from
Dec 7, 2017
Merged

Refactored python script #13

merged 13 commits into from
Dec 7, 2017

Conversation

conqp
Copy link
Contributor

@conqp conqp commented Nov 15, 2017

I took the liberty to completely refactor the python script native.py.
I tested it with the dev-version of firefox.

Changes

  • Broke down code into several separate functions (divide and conquer).
  • Made use of some useful python structures like namedtuple and Enum.
  • Made variable naming and indentation PEP8 compliant.

If you have any questions, please contact me.

@conqp
Copy link
Contributor Author

conqp commented Nov 16, 2017

1bc6781 addresses #5

@conqp conqp closed this Nov 24, 2017
@GKFX
Copy link
Owner

GKFX commented Nov 25, 2017

@conqp Thank you very much for all this! I'm sorry I haven't been able to review it sooner but as soon as I'm available I'll try to get it merged if you're still happy with it.

@GKFX GKFX reopened this Nov 25, 2017
@conqp
Copy link
Contributor Author

conqp commented Nov 26, 2017

@GKFX Yes sure. I'm sorry that I misinterpreted the long silence here for a lack of interest.

@dariuskl
Copy link

dariuskl commented Dec 4, 2017

@conqp
I was looking into a way of decorating new windows and just saw your work here. Just a hint: maybe it would be a more resource-conscious approach to add a listener for the window creation event in the extension itself and send a native message from there instead of having the script running in the background.

@GKFX GKFX merged commit f18ac75 into GKFX:master Dec 7, 2017
@conqp
Copy link
Contributor Author

conqp commented Dec 12, 2017

@dariuskl I am not too well versed with Firefox plugin development. If you have a better solution for decorating new windows (and probably any other solution is better than my hack), I think it'd be great if you contributed your ideas in form of code. :)

@GKFX
Copy link
Owner

GKFX commented Dec 12, 2017

@conqp One of the major issues with this PR is that it removes the titlebar from all windows owned by Firefox, not just the "real" browser windows that display a webpage. It's possible that @dariuskl's approach is the correct solution here, since the window created events seem to fire only for proper windows.

@conqp
Copy link
Contributor Author

conqp commented Dec 12, 2017

@GKFX Well, the original script just decorated the first of the windows of the firefox process.
I don't think that decorating all of the firefox process's windows on startup in itself is a problem, since there should be just one such window (I need to verify this).
I agree however, that this is a problem on the continuous decoration, which is also, why I disabled it as per default.
I completely agree, that @dariuskl's approach sounds way better (which I already stated).
If you feel uncomfortable with this testing code in my PR, I can remove it.

@GKFX
Copy link
Owner

GKFX commented Dec 12, 2017

I'm very happy to work with what you've done; my original script was quite limited and did only decorate one window. I was just suggesting that the window created events might be the solution to identifying which windows are appropriate to decorate. Although since there doesn't appear to be a straightforward link between the Window objects seen by the extension and the windows seen by the Python script (the IDs and even the initially reported titles are different) I'm considering just identifying browser windows on the basis that they contain a suitable string (such as "Firefox" or "Nightly") and are owned by the correct process.

@conqp
Copy link
Contributor Author

conqp commented Dec 12, 2017

I also see the problem with identifying the main windows of firefox.
I will have a closer look at it when I find the time.
PS: With I can remove it I was referring to the testing code, not the entire PR request. ;)

@dariuskl
Copy link

I think it'd be great if you contributed your ideas in form of code. :)

Fair enough. :-) I opened pull request #19, which includes the listener. New windows are correctly (un-)decorated. However, still all windows are affected.

Regarding that issue, I don't think there is a nice way of linking the Firefox windows to window manager IDs. Maybe it would be a solution to temporarily insert "markers" into the title with which browser windows can be differentiated? I'm not so sure yet whether this is a good idea, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants