Skip to content

Adds winprop preferredWidth to set default window width by wm_class (supports 'px' or '%' values)#453

Closed
jtaala wants to merge 8 commits intopaperwm:developfrom
PaperWM-redux:preferred-width-step-winprop
Closed

Adds winprop preferredWidth to set default window width by wm_class (supports 'px' or '%' values)#453
jtaala wants to merge 8 commits intopaperwm:developfrom
PaperWM-redux:preferred-width-step-winprop

Conversation

@jtaala
Copy link
Copy Markdown
Collaborator

@jtaala jtaala commented Dec 25, 2022

Adds user configurable window property (settable in user.js for example) preferredWidth. This property allows setting an initial window width. Accepts a String value with unit px or % (e.g. "50%", "500px", etc).

See README.md changes in this PR for more details (under section Winprops).

This approach builds on / extends (and relates to) #304. I wouldn't say it fixes that PR(?) since it only addresses the width (and not the vertical height).
I'm going to say this fixes #304 since the discussion in that issue focuses on width (with some comments re height being tricky). Can create another issue for setting preferredHeight if there is appetite for that).

Thoughts and feedback welcome!

Updated README.md and added table with some Winprop descriptions.

Added special case Winprop value for preferredWidthStep = 3 which maximises the window
on creation.
Defining a ratio seems cleaner and is then independent of the useful
width steps (which is user configurable).
@jtaala jtaala changed the title Adds winprop preferredWidth and preferredWidthStep. Adds winprop preferredWidth and preferredWidthRatio. Dec 27, 2022
@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Dec 28, 2022

@Gelbana @tadfisher @ccope @smichel17 - here's another quick fix to review when someone has time.

We could even combine this functionality into one winprop preferredWidth and have it handle both pixel or ratio values (e.g. if value is <= 1, treat as ratio).

Thoughts?

Copy link
Copy Markdown
Member

@smichel17 smichel17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think combining them into one setting is a good idea.

From the user's perspective, I think it's a simpler interface. I would suggest defining it as a string.

This way, we keep the flexibility to add different units of measurement in the future, without complicating the interface further. The numerical behavior could be allowed but should be undocumented/undefined, since it would likely change if we added a new unit.

For the internal representation, it's easier to reason about when the property either exists or doesn't vs also having to handle the 3rd case of when both exist (this gets worse with additional units). The code itself isn't more complicated, but it increases the chance of some other part of the code accidentally depending on the order of priority, or something like that.

  • For now, I think an internal representation as a number, with the fraction/pixel distinction you mentioned, is good. If more units are added, it would probably make sense to switch to an object with the shape { value: number, unit: string }.

  • I would suggest parsing the user input to the internal representation as soon as it is defined (i.e. in the defwinprop function).

@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Dec 29, 2022

@smichel17, yep, that makes a lot of sense. I hadn't considered other units, but yes that is a possibility.

If we make it a string input then there's really no reason why we can't parse units as part of that - e.g. like css 600px, or 4em etc. with the parsing like:

  • if a unit is defined (e.g. 400px or 10em') then convert value into pixels (and normal >1 interpretation applies);
  • if no unit is defined AND value is > 1 - treat as pixel value;
  • else treat is as ratio value;

Do you think that might cover current (and future perceicable) use cases for this?

@smichel17
Copy link
Copy Markdown
Member

smichel17 commented Dec 29, 2022

  • Just to be explicit (text is hard), I'm not suggesting you ought to actually add support for other units now. I mean, it's fine if you feel like it, but I think px and % will cover the vast majority of use cases.

  • I think that sounds good. To nitpick, I could add the edge case: values below 0 are invalid and will be discarded. (edit: I'm talking about internal representation here. For external, I think officially only string values ending in px or % should be "officially" supported. Actually maybe it would be better not to support numeric values at all; people have a tendency to complain when things break even if they weren't supposed to work in the first place…).

  • I don't think it covers all possible future use cases. That's a tall order. Fortunately, we can change the internal representation later if we need to, so covering all possible future use cases is not required. It doesn't even need to cover all present use cases; just the documented ones ;)

    • When I was retroactively trying to think of use cases to justify my gut-feeling that the width should be defined as a string, the one I came up with was something like a ratio, where the width would be based on the height (e.g. 8:5).

@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Dec 29, 2022

Nitpicking is fine! (since I don't really like nits!). I agree though - I'll update external to support % and px and update documentation for this.

One thing though - you'll note a few other PRs like #456 and #454 do use standard notation for ratios (0.5 as opposed to 50%)... my undergrad was mathematics so I prefer standard notation for ratios (but then again, I'm biased there); but I do like the fact that px and % and string input is more akin to css's approach there.

Not sure what you think about consistency for those other PRs. Thoughts? (edit: at this stage, I'll just implement the changes there and we can discuss those other PRs when needed).

@smichel17
Copy link
Copy Markdown
Member

smichel17 commented Dec 29, 2022

I agree that consistency is good. I personally like standard notation. The "software engineer" part of my brain screams ONLY ALLOW ONE WAY TO DO IT AND MAKE UNITS EXPLICIT.

I'll sleep on it. As long as it's parsed to the internal representation right away, it's probably fine.
I think it's only a question of, "Is there a more useful thing we could do with numeric inputs later?"
It's easy to add support later; hard to remove it once people use it.

with units "px" or "%" (e.g. "50%" or "500px" etc.)
@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Dec 29, 2022

Thanks @smichel17! See changes in latest commit in this PR.

Yes, this is a much cleaner approach. Currently accepting % or px units - anything else (or no unit) will not do anything and write a warning to extension log. Likewise for any value input <= 0 (or no value input given).

Please re-review.

P.S. I did end up going with an internal {value, unit} object structure that is processed in defwinprops - good suggestions!

@jtaala jtaala requested a review from smichel17 December 29, 2022 04:48
@jtaala jtaala changed the title Adds winprop preferredWidth and preferredWidthRatio. Adds winprop preferredWidth Dec 29, 2022
jtaala added a commit to PaperWM-redux/PaperWM that referenced this pull request Dec 29, 2022
…ay (in

extension settings) to percentage format (e.g. `30` instead of `0.3`).
…exists (avoids duplicates).

Means if multiple for the same wm_class are defined (for some strange
reason) then will take the last one.  Updated comments and the WARNING
for unvalid units.  Lastly, being more strict on the units (previously
was lowercasing them so 'PX', 'Px', 'pX' would still work...).
@jtaala jtaala changed the title Adds winprop preferredWidth Adds winprop preferredWidth to set default window with by wm_class (supports 'px' or '%' values) Jan 1, 2023
@jtaala jtaala changed the title Adds winprop preferredWidth to set default window with by wm_class (supports 'px' or '%' values) Adds winprop preferredWidth to set default window width by wm_class (supports 'px' or '%' values) Jan 2, 2023
jtaala added 3 commits January 2, 2023 14:33
new `preferredWidth` window property.  Left reference to not currently
being able to set the window height.
jtaala added a commit that referenced this pull request Jan 21, 2023
… settings (#454)

* Added layout() call to show() - needed to readjust minimap position
independent of MINIMAP_SCALE (preparation for setting scale to user
option).

Added "minimap-scale" to schema (and recompiled schema).  Minimap.js now
uses "prefs.minimap_scale" from schema.

Added "Mini-map scale (size)" GtkSpinButton to settings.ui and
implemented binding in prefs.js.

* Removed period from tooltip description to align with gnome human
interface guidelines.

* Set right-side window preview's x position directly (using the
used scale and clone.width instead of 'get_transformed_size()').

* Implemented fix for issue #457.  Now uses user-defined mini-map scale
value for StackOverlay window previews.

* For consistency with other PRs (like #453) changed value display (in
extension settings) to percentage format (e.g. `30` instead of `0.3`).
@jtaala jtaala closed this in #463 Jan 31, 2023
jtaala added a commit that referenced this pull request Jan 31, 2023
…463)

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


https://user-images.githubusercontent.com/30424662/211422647-79e64d56-5dbb-4054-b9a6-32bf3194b636.mp4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Any way to set per wmclass preferences for default size

2 participants