-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code style cleanup, support for window stealing, support for right-click for settings #300
base: master
Are you sure you want to change the base?
Conversation
Thanks. I wanted to try it, and I noticed that you didn't update the Makefile. Just list the required files, changing those you renamed and adding the additional ones in the EXTRA_MODULES variable, to start. That should be enough. |
Oops, you're right. :/ |
Fixed. |
I see that you just made some major changes! I'd be happy to do the work to merge them into my pull request, but I want to make sure you actually have interest in it. I do think it really does a lot to clean up your codebase and make it friendlier to contributors. |
Hi, I just fix the inevitable bug after the latest release, no major changes, nor any is planned to land soon. I've just had the time to run your version (Settings.ui is still missing in the Makefile by the way). I'm looking right now to your changes, but it will take me some time. Although merging the few changes should be trivial, there's no need for you to do it right now. Regarding your changes, the first impact with the "windows stealing" functionality has been "now what do I write here? What's a the WM_CLASS...", while I'm not able to access the settings right clicking on the dash. Michele |
convenience.js
Outdated
const Gettext = imports.gettext; | ||
const Gio = imports.gi.Gio; | ||
const Clutter = imports.gi.Clutter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be before (in alphabetic order).
At the moment I'm trying to split the code reorganization (which once refined I want to merge as soon as possible) from the additional features (which might require more thinking). This will also help backporting to previous gnome shell versions. The code really required some clean up and a better consistency: very nice work. Out of curiosity, did you use any tool to enforce a specific code style? |
Hey, I see that you are commenting on actually an older version ... I might various fixes since then, please check my latest master. Not sure if github is having a bug here... I did all the code style by hand because I didn't want to mess anything up with an automatic tool. It was a lot of work. :/ The question about how window stealing works: you can use the For example, if you are say launching Minecraft with a shell script, then you need to steal the resulting window, otherwise you get a mess on your dock. For the latest version of Minecraft the WM_CLASS is "Minecraft 1.9". I've also tested it with running remote applications using |
Hi, A quick update: I think I have condensed your style and reorganization changes (included your latest ones) in this commit 6a651a7, isolating the changes related to additional features in the following commits. I still have to double check I haven't left dirt around and that there are not major unwanted changes left, and consider few other style changes. After that this will be merged into master (after proper rebase) and I will look at your additional features in more detail. |
Looks good! I guess I should have done it this way, too, but the way it happened was that working on one thing turned into another... |
intellihide.js
Outdated
this._topApp = this._tracker.get_window_app(topWindow); | ||
// If there isn't a focused app, use that of the window on top | ||
this._focusApp = this._tracker.focus_app || this._topApp | ||
|
||
windows = windows.filter(this._intellihideFilterInteresting, this); | ||
|
||
for(let i=0; i< windows.length; i++) { | ||
|
||
for (let i in windows.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It tool me a while to find the source of a bug: can you confirm that this is unintended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, unintended! Oops, I really did not intend to change any code. Probably it was a bad copy/paste. I apologize for wasting your time trying to find the bug. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. The reason It's taking a while to merge the changes is that I'm trying to to be sure to not introduce bugs with these huge commits, as it will be very difficult to spot the difference in the future.
Sorry if it's taking me a long time to merge, I haven't had much time lately. |
There is no rush at all. :) Please note that I made some more small fixes to master. Basically, anything that you want to fix just let me know and I can do that, so that in the end you can just push "merge" and have it done. |
I've merged into master the first big code refactoring commit [f9b4bab]. I'm confident everything is consistent and there are no bugs! I know I will be proven wrong, but I don't want to leave it unused for too long or it will be lost. Thanks again for the great work. Additional changes will follow. |
Terrific! I'm traveling now but will double check next week.
|
Well, it's been a long time since I touched this. But I wonder if you're still interested at all in this feature? Right now there are various launchers that don't behave properly in the dock: you launch them with one icon, and they create new icons for every window. I don't think it's hard to find a solution by gathering all such windows by their window name. As it stands, this is a glaring bug in otherwise terrific software. If you want to see an example of such a launcher, try installing the itch.io client. Add itch.io to your favorites, and you'll see that when you launch it, it creates a new icon. |
Hi @tliron, sorry for the late reply and thanks for pinging on this issue. I understand the problem and also experience similar issues on a daily basis with windows not being grouped correctly. I'm not quite sure if this is the solution to the problem, but then, what else can we do at the extension level? I'm wondering if we should just have a simple interface and some documentation for it. At the moment the GUI is nice up to the point it asks for the windows classes, which most people -- me included -- have no idea how to easily obtain. A nice user-friendly interface would probably require way too much effort, but it could be considered together with issue #445, as a general way to workaround this and other windows management bugs. I don't have much time to dedicate to this issue now, so I can't make promises. I would appreciate if you could just isolate your patches and possibly rebase on master so that in future it will make it easier for me to go back and work on it (right now they still mixed with your work on code clean up). |
Sounds good. OK, so I think it should be separated into two PRs:
|
That would be ideal, thank you. |
This seems like a very massive pull request, but that's only because of the code-style cleanup. :) Actually, the change is just adding one file and a few smaller interventions.
Cleanup involved:
Window stealing feature:
IMPORTANT: Let me say that I'll be happy to continue supporting this feature. And actually generally help you fix bugs in Dash to Dock, now that I know the codebase well. I'm not just dumping a feature on you, I really want to collaborate and help. :)
The code is all in windows.js, however it also affects some code in dash.js and icons.js. Specifically, instead of calling app.get_windows, we are now calling Windows.getAllWindows which knows how to handle window stealing (if enabled). We are additionally adding some simple hooks to setting changes to make sure to refresh the icons.
Right-click for settings:
This adds to ability to right-click anywhere on the dash (outside the icons) and get a simple popup menu that lets you run the settings. This is very useful for users who don't have the applications icon enabled (otherwise they have no easy way to get to settings).
The code is in dash.js line 81. Very straightforward!