Code style cleanup, support for window stealing, support for right-click for settings #300

tliron · 2016-03-27T19:38:00Z

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:

Using more and smaller JS files, organized by "feature" (to be more like the Shell's codebase).
Standardizing on one specific JS style (there were many different styles before: different use of spaces, indents; sometimes single line blocks had brackets and sometimes not; different styles of comments; different standards for camel-case; various conventions for GObject class names; and more).

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 feature is disabled by default, but users can enable it in prefs, in the "behavior" pane.
When enabled: an extra "Window Stealing" entry is added to the app icon popup menu. It entry will open a simple configuration dialog box that lets you enter a list of WM_CLASS names (or leave empty to remove stealing).
Once set, those windows will now be "stolen" by this icon -- the icon show in the active style if any of these windows are open, and will have the correct number of dots. Click to minimize/maximize will also take these windows into consideration. Relatedly, the extra running app icons will not appear for those windows.
Known bug: if you de-configure window stealing for an icon, the icon will immediately stop stealing, but the extra running icons for those windows will not reappear until restarting the Shell. This is rather hard to solve, and is also special edge case that would likely only appear during testing, so I don't think it should affect releasing the feature.
Another tiny bug: window minimizing/maximizing animations originate at the location of the now-invisible stolen app icon on the dash, rather than the stealing one. This doesn't affect usability, of course, but is something I will try to improve.

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!

… settings

micheleg · 2016-03-27T19:46:57Z

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.

tliron · 2016-03-27T19:52:22Z

Oops, you're right. :/

tliron · 2016-03-28T03:35:59Z

Fixed.

tliron · 2016-04-03T07:06:51Z

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.

micheleg · 2016-04-03T19:05:59Z

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

micheleg · 2016-04-03T19:17:25Z

convenience.js

 const Gettext = imports.gettext;
 const Gio = imports.gi.Gio;
+const Clutter = imports.gi.Clutter;


Should be before (in alphabetic order).

micheleg · 2016-04-03T23:10:05Z

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?

tliron · 2016-04-03T23:35:36Z

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 xprop WM_CLASS to find out the class of a window and experiment with that. Ideally I would want that incorporated into the dialog, but that's more work for the future.

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 ssh -X.

micheleg · 2016-04-05T00:14:56Z

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.

tliron · 2016-04-05T01:41:25Z

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...

micheleg · 2016-04-09T23:44:55Z

intellihide.js

                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) {


It tool me a while to find the source of a bug: can you confirm that this is unintended?

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. :/

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.

micheleg · 2016-04-17T17:22:34Z

Sorry if it's taking me a long time to merge, I haven't had much time lately.

tliron · 2016-04-17T17:48:13Z

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.

micheleg · 2016-05-11T22:25:33Z

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.

tliron · 2016-05-12T14:39:09Z

Terrific! I'm traveling now but will double check next week.
On May 12, 2016 01:25, "Michele" notifications@github.com wrote:

I've merged into master the first big code refactoring commit [f9b4bab
https://github.com/micheleg/dash-to-dock/commit/f9b4bab12b67179fbdd325b1822dfbdd7392a576].
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.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#300 (comment)

tliron · 2017-03-24T05:06:55Z

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.

micheleg · 2017-03-29T21:13:01Z

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).

tliron · 2017-03-30T18:22:03Z

Sounds good. OK, so I think it should be separated into two PRs:

One PR that implements the feature without a GUI. To enable/test it you'll need to edit the gsettings directly.
Assuming everything works with that, we can move to a second PR that adds a nice GUI.

micheleg · 2017-03-31T20:52:57Z

That would be ideal, thank you.

tliron added 6 commits March 25, 2016 15:48

Code reorganization; begin work on "window stealing"; right click for…

96cca19

… settings

More cleanup; use GSettings for window stealing

de2e056

Window stealing settings

4ef94fb

Add support-window-stealing flag to prefs

befc8af

Improve window stealing dialog

77cd6c1

Small style fix

2c35119

Fix makefile

7c5cd9d

tliron added 2 commits March 27, 2016 23:29

xgettext fixes

3b210a4

Use pipes instead of spaces to separate WM_CLASS

21f234c

micheleg reviewed Apr 3, 2016
View reviewed changes

Some fixes

eaaf24a

micheleg reviewed Apr 9, 2016
View reviewed changes

Fix windows iteration

41a9911

micheleg mentioned this pull request Jun 25, 2016

Right click on empty space in extended dock could open settings menu #336

Open

micheleg force-pushed the master branch from c01e962 to a168e2f Compare March 25, 2017 01:33

micheleg mentioned this pull request Mar 29, 2017

With isolate workspaces enabled, prevent multiple instances of a specific application from launching #445

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code style cleanup, support for window stealing, support for right-click for settings #300

Code style cleanup, support for window stealing, support for right-click for settings #300

tliron commented Mar 27, 2016

micheleg commented Mar 27, 2016

tliron commented Mar 27, 2016

tliron commented Mar 28, 2016

tliron commented Apr 3, 2016

micheleg commented Apr 3, 2016

micheleg Apr 3, 2016

micheleg commented Apr 3, 2016

tliron commented Apr 3, 2016

micheleg commented Apr 5, 2016

tliron commented Apr 5, 2016

micheleg Apr 9, 2016

tliron Apr 9, 2016

micheleg Apr 9, 2016

micheleg commented Apr 17, 2016

tliron commented Apr 17, 2016

micheleg commented May 11, 2016

tliron commented May 12, 2016

tliron commented Mar 24, 2017

micheleg commented Mar 29, 2017

tliron commented Mar 30, 2017

micheleg commented Mar 31, 2017

Code style cleanup, support for window stealing, support for right-click for settings #300

Are you sure you want to change the base?

Code style cleanup, support for window stealing, support for right-click for settings #300

Conversation

tliron commented Mar 27, 2016

micheleg commented Mar 27, 2016

tliron commented Mar 27, 2016

tliron commented Mar 28, 2016

tliron commented Apr 3, 2016

micheleg commented Apr 3, 2016

micheleg Apr 3, 2016

Choose a reason for hiding this comment

micheleg commented Apr 3, 2016

tliron commented Apr 3, 2016

micheleg commented Apr 5, 2016

tliron commented Apr 5, 2016

micheleg Apr 9, 2016

Choose a reason for hiding this comment

tliron Apr 9, 2016

Choose a reason for hiding this comment

micheleg Apr 9, 2016

Choose a reason for hiding this comment

micheleg commented Apr 17, 2016

tliron commented Apr 17, 2016

micheleg commented May 11, 2016

tliron commented May 12, 2016

tliron commented Mar 24, 2017

micheleg commented Mar 29, 2017

tliron commented Mar 30, 2017

micheleg commented Mar 31, 2017