Adds user configurable Winprops tab to PaperWM extension settings ui

[jtaala]

Fixes #462, closes #453, closes #304.

This PR adds a new tab to the PaperWM extensions settings ui that allows users to:

• set default window properties for a window class (app etc.);
• settings are immediately applied (no need to login/logout of gnome as is needed for the current approach);
• set window (identified by wm_class or title) to open on scratch by default;
• set a preferred window (identified by wm_class or title) width - supports both % and px input values;

Notes:

• the preferredWidth property is ignored if a window is set to scratch_layer;
• Winprop entries that have no wm_class or title input will not be applied/persisted (they won't show when you open the preference settings next time);
• this does not impact the current approach of setting winprops in user.js configuration (which you can still do);
• modelled after keybinding settings ui (and fits in nicely with it);

Feedback super welcome!

See it in action in the video below (click the play button):

paperwm-winprops-settings.mp4

[jtaala]

@smichel17 @ccope @Gelbana @tadfisher - requesting thoughts, code review and/or testing (pretty pretty please!).

This PR allows users to set their custom winprop settings in the extension settings ui and have them apply immediately (no more needing to restart gnome in X11 or gasp logout/login in wayland).

[jtaala]

Hey @Gelbana, hope you're doing well! I added a request for review from you as I saw you had worked on some winprop related stuff previously. Keen to have you look through this PR if you're keen / when you're able to!

[Gelbana]

I can't see any obvious issues (but I'm not too experienced with JS so take that with a grain of salt).

One thing I found was an inconsistency with how the user.js and UI defined winprops interact when both contain rules that match the same window.

For example:

• scratch enabled in user.js -> restart shell -- reflected correctly (window in scratch)
• scratch enabled in user.js -> preferred width enabled in UI -- UI changes not reflected in placement (still on scratch)
• scratch enabled in user.js -> preferred width enabled in UI -> restart shell -- UI changes take priority over user.js (placed with preferred width)
• scratch enabled in user.js + preferred width enabled in UI -> remove UI setting -- user.js defined is not reflected (still placed with preferred width)
• scratch enabled in user.js + preferred width enabled in UI -> remove UI setting -> restart shell -- user.js reflected correctly (in scratch)

Clarification regarding the priority of user.js and UI defined settings may be useful to add to the readme.

[jtaala]

I can't see any obvious issues (but I'm not too experienced with JS so take that with a grain of salt).

One thing I found was an inconsistency with how the user.js and UI defined winprops interact when both contain rules that match the same window.

Thanks so much @Gelbana! - good points too re cases where both user.js and ui have winprops - currently I'm detecting if a winprop exists and updating that winprop... but as you can see it can get a bit inconsistent depending on the order (restart shell, do ui operations).

To make things more consistent - maybe we have user.js take priority (e.g. settings defined winprop won't do anything if a user.js winprop already exists... that's certainly much cleaner.

Either that, or the other way around - have settings defined winprops always take precedent. The advantage of that is that it's much easier to change the settings winprops (applies instantly) rather than changing user.js winprops (which require a shell restart / login-logout).

what do you think? Either way I'll update the README.md to clarify preference order.

[jtaala]

Hey @Gelbana,

I've now made the changes discussed above. To be consistent, winprops defined in the settings ui take precedence over winrprops defined in user.js.

We do this by ordering winprops defined in the settings ui before those added via user.js. This just means that they will be picked up first in the find_winprop function (and hence take precedence).

Feel free to do another test. Once you're happy could you please then approve this PR so it can be merged into develop.

Thanks!

P.S. I also updated the README.md to winprop order of precedence.

[jtaala]

Thanks @Gelbana!

[smichel17]

I think this is good, but incomplete.

• I'd call the tab something like "Per-Window Rules". Winprops is not a name that ~anybody is going to understand right away.
• The wm_class field should be renamed to "Application" or maybe "Application (wm_class)"
  • "Add Winprop" can be renamed to "Add Rule".
• It's missing a field for wm_title. I suggest "Window Title" or just "Title", optionally also with "(wm_title)" tacked on the end.
• Finding the wm_class is pretty finnicky/annoying to me right now. You have to open looking glass, pick the window, and then go trawling through the properties in the looking glass console. Also if I remember correctly you can't easily copy-paste out of the console, so you need to manually retype. It's great functionality but frustrating to set up.
  • Fortunately, PaperWM already know the wm_class (and wm_title, although you can usually find that in the title bar). I suggest that there should be a way to copy the wm_class and wm_title from an open window into a new winprop entry. Maybe the "Add Rule" button can open a drop-down that shows the open windows, and also has an entry for "Empty rule".

Also, with regard to user.js not reloading— I think we should also try and solve that directly. Maybe a button to reload user.js (or even the entire extension?). But I can open a separate issue about that.

[jtaala]

Thanks @smichel17,

Just wondering if you're running what was merged? "Title" was added a few weeks back in this commit . I suppose I should update the video to reflect that.

Also, with regard to user.js not reloading— I think we should also try and solve that directly. Maybe a button to reload user.js (or even the entire extension?). But I can open a separate issue about that.

Yeah, I toyed with moving the Tiling.defwinprop method calls to the enable() lifecycle hook - problem is that by design, the init() hook (where documentation suggests having the Tiling.defwinprop and where users will be putting them), is only run once on startup (e.g. when a user logs in gnome fresh / or in X11 restarting gnome, and not when the extension is reloaded etc.). There's other code in init methods of the modules that should only be run as such. Wayland has it's own issues - in that you can't actually "restart" wayland gnome, other than a full logout/login.

enable is the lifecycle hook/method that is run on extension reloads (and also apparently when unlocking screen etc.).

Anyways, a button to reload user.js sounds like a good idea - however, if users have other methods (it's just js btw and they have full access to all the modules etc.) in the init (which by definition is only designed to be run once) there may be issues etc. from running init code that should only be init with gnome loading etc.

I agree, create a new issue for this one as it may require some refactoring to address this.

[jtaala]

Finding the wm_class is pretty finnicky/annoying to me right now. You have to open looking glass, pick the window, and then go trawling through the properties in the looking glass console. Also if I remember correctly you can't easily copy-paste out of the console, so you need to manually retype. It's great functionality but frustrating to set up.

• Fortunately, PaperWM already know the wm_class (and wm_title, although you can usually find that in the title bar). I suggest that there should be a way to copy the wm_class and wm_title from an open window into a new winprop entry. Maybe the "Add Rule" button can open a drop-down that shows the open windows, and also has an entry for "Empty rule".

I really like this - will make it a ton easier as, yes, it's often a pain find wm_class / title etc. The other idea is could add a item to copy wm_class or title in the paperwm drop-down menu when right-clicking on window titlebars or pressing alt+space (where you see the scratch option etc.).

[smichel17]

Just wondering if you're running what was merged?

Caught me 😆. I did a pretty quick review and only looked at the comments here (not even the code).

drop-down menu when right-clicking on window titlebars

I think it's probably more convenient than my suggestion for creating the winprop rules. However, it's probably not something you're going to use most of the time, so it would clutter up the menu. For that reason, I'd rather not do it, unless you can only show the option when the settings dialog is open (in which case, yeah, opening the settings, then right click on another window -> "create rule for this window" would be awesome).

init

I think it would be a good idea to discourage people from putting code in init unless it's strictly necessary.

Partially we're just running afoul of gnome's extension design here (lack of any extension api, requiring extensions to take care of disabling themselves, not having a way to start fresh). There's some extent to which I don't think it's the best use of our effort to work around gnome's limitations. That also applies to winprops a little— if looking glass were easier to use, we wouldn't need to solve this problem.

[jtaala]

Caught me laughing. I did a pretty quick review and only looked at the comments here (not even the code).

All good!

For that reason, I'd rather not do it, unless you can only show the option when the settings dialog is open (in which case, yeah, opening the settings, then right click on another window -> "create rule for this window" would be awesome).

Yeah, I agree, looking glass is a pain, and has some weird behaviour for me. Anyways, showing a list of windows in the settings ui (for user to select and have it populate wm_class or title) seems a better approach here.

I think it would be a good idea to discourage people from putting code in init unless it's strictly necessary.

Agree - never liked putting these in init - and definwinprop just doesn't feel right there (which is partially why I wanted to add this stuff to settings.ui.