Skip to content

Adds user configurable tiling edge margins to PaperWM extension settings ui#467

Closed
jtaala wants to merge 6 commits intopaperwm:developfrom
PaperWM-redux:tiling-edge-margin-user-settable
Closed

Adds user configurable tiling edge margins to PaperWM extension settings ui#467
jtaala wants to merge 6 commits intopaperwm:developfrom
PaperWM-redux:tiling-edge-margin-user-settable

Conversation

@jtaala
Copy link
Copy Markdown
Collaborator

@jtaala jtaala commented Jan 22, 2023

Closes #364

This PR adds user configurable tiling edge margins to the settings ui. Edge margins are the minimum margin from screen edges to the leftmost and rightmost window edges. Currently the edge margin is set to 0px (by design).

Edge margins were discussed in #364, with some users preferring having an edge margin. This PR allows users to:

  • select their preferred margins;
  • preview the edge margins as the settings is changed;

Video demonstration of changing edge margins:

paperwm-tiling-edge-margin.mp4

NOTE: this PR has been implemented in the PaperWM-redux fork, which you can install if you want this, or any of my other open PRs.

@jtaala jtaala requested a review from hedning January 22, 2023 15:21
@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Jan 22, 2023

Hey @hedning, this was a while back but you responded regarding the design decisions re edge margins back in #364 - keen to get any thoughts re this PR which makes them user configurable.

@jtaala jtaala requested a review from smichel17 January 22, 2023 15:24
@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Jan 29, 2023

@smichel17 - given our conversation in #454 and #456 re user settings - what do you think of this one?

I think this one definitely fits - as it's a margin.

Also, interestingly I've noticed that it's one of the first things that seems "off" to new users, although it's intended behaviour to let users know when they are a tiling edge. I prefer 5px though - so I can see my nicely styled border but still see when at the tiling edge.

Anyways, we could leave this one there and move or remove the Useful window widths/heights settings to cleanup it up a bit (removing the widths/heights setting and users can use dconf to set those ones).

@smichel17
Copy link
Copy Markdown
Member

I think this is one we can fix without a setting. I'll reply in #364

@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Jan 30, 2023

I think this is one we can fix without a setting. I'll reply in #364

Thanks @smichel17! I don't think it's a bug though, it's a feature! :-). I added a comment to #364 outlining why I chose to have it as a simple user-defined preference setting (which is fit nicely in the settings ui with the margin settings).

@Lythenas
Copy link
Copy Markdown
Collaborator

I tested this and it seems to work without issues. I set the margin to 5px and I also have the other margins set to 5px. So there is no space between the borders and the edge of the screen.

@jtaala
Copy link
Copy Markdown
Collaborator Author

jtaala commented Mar 15, 2023

As mentioned in #488, closing this in favour of #488.

@jtaala jtaala closed this Mar 15, 2023
jtaala added a commit that referenced this pull request Mar 15, 2023
…sed no margin to be shown (#488)

Closes #364.

This PR was created since @basdelfos develop branch/fork isn't available
anymore :-(

Please see #364 for details of this. It has been a long time coming.
Given the merging of #471 (an improved way to see the window position),
and also the merging of #473, having no margin on the left/right tiling
edge no longer provides the original benefit (of seeing when you're at
an tiling edge).

This simply removes the cases where no left/right margin would be shown
- which also cleans-up/simplifies this area of code (thanks @smichel17
).

Note: we will close #467 as it was originally a way to set this
separately. However we will be closing that in favour of this one.
jtaala added a commit to PaperWM-redux/PaperWM that referenced this pull request Mar 16, 2023
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.

3 participants