Skip to content
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

Show persistent viewport window position indicator in topbar (completes #471) #476

Merged
merged 28 commits into from Mar 11, 2023

Conversation

jtaala
Copy link
Collaborator

@jtaala jtaala commented Feb 21, 2023

Fixes #471

This PR adds a window position indicator bar as described in #471.

The bar style can be overriden with the css class paperwm-window-position-bar.

I've also added a schema property to enable/disable showing the position bar.
You can disable it from the command line with:
dconf write /org/gnome/shell/extensions/paperwm/show-window-position-bar false
and enable it with
dconf write /org/gnome/shell/extensions/paperwm/show-window-position-bar true

See the window position bar (topbar blue coloured segment indicating window tiled position) in the video below:

paperwm-window-position-bar.mp4

Feedback welcome!

The indicator might not be wanted by some users - so, it might be worth adding a toggle in the paperwm preference ui.

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.

forceTransparency method in topbar which ensures transparency is
maintained so window indicator can be seen.
@jtaala jtaala marked this pull request as draft February 21, 2023 14:13
@Lythenas
Copy link
Collaborator

I think this looks very good.

But I have a comment about the naming: "Window indicator" is really generic and does not really tell you anything. So at the very least we should document it properly in the code (and probably readme) what exactly it is. Also I guess it looks effectively like a scroll bar, so maybe that is a better name for it?

Also maybe we need some discussion about the default color, but it is easily configurable so maybe not that important. The default color of the window selection comes from the .tile-preview class, which is defined in the gnome theme. Also if I remember correctly on one gnome version I was able to select a color in the gnome settings and it also changed the paperwm window selection color, but on gnome 43.2 I can't find that setting anymore. I guess what I'm getting at is: What should be the default color? The same as the window selection or another one?

@jtaala
Copy link
Collaborator Author

jtaala commented Feb 21, 2023

Thanks @Lythenas ,

But I have a comment about the naming: "Window indicator" is really generic and does not really tell you anything. So at the very least we should document it properly in the code (and probably readme) what exactly it is. Also I guess it looks effectively like a scroll bar, so maybe that is a better name for it?

I guess it does look like a scroll bar, my only issue with referring to it as a scroll bar is that that name has some meaning behind it - e.g. users will likely try to click on it and drag it left/right (like s a scroll bar) . What about something like viewport position indicator? or maybe a mix: viewport position bar? Actually that last one sounds pretty good(?). Open to other names/ideas.

Also maybe we need some discussion about the default color, but it is easily configurable so maybe not that important. The default color of the window selection comes from the .tile-preview class, which is defined in the gnome theme. Also if I remember correctly on one gnome version I was able to select a color in the gnome settings and it also changed the paperwm window selection color, but on gnome 43.2 I can't find that setting anymore. I guess what I'm getting at is: What should be the default color? The same as the window selection or another one?

I think using tile-preview style/colors as the default color is good here and thinking about it should have been the default I went with. I just chose green because I wanted it to stand out a bit for showcasing what this thing is. I'll change the default style and throw up another video tonight to see what it looks like.

Cheers!

@jtaala
Copy link
Collaborator Author

jtaala commented Feb 22, 2023

Thoughts? should the position bar be shown when there is only one window? or only when there is at least two windows?

Showing with one window seems a bit much? (not too useful?).

@jtaala jtaala marked this pull request as ready for review February 24, 2023 13:04
@jtaala jtaala self-assigned this Feb 25, 2023
…dget (so position bar colour not obscured by topbar).

Original implementation had the topbar transparency set and the position
bar behind it.  This was okay, but the position bar colours were
obscured/darkened by the topbar.  This new implementation has the
position bar sitting on top a backdrop widget (which has some
transparency).  Now the position bar colour shows unobscured.
deprecated/no-longer-used css class from stylesheet).  Added
window-position-bar-backdrop css class so it can be styled by users if
wanted.
@jtaala jtaala requested review from Lythenas and removed request for Gelbana March 6, 2023 21:42
@jtaala
Copy link
Collaborator Author

jtaala commented Mar 7, 2023

Hey all! I've been testing this in anger the last two weeks and has been working great.

I'm planning on merging this in one week, please post any comments or thoughts re this PR if you have any.

Copy link
Collaborator

@Lythenas Lythenas left a comment

Choose a reason for hiding this comment

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

Overall I think the code looks good.

I will try this out for the next couple of days, before approving.

README.md Outdated Show resolved Hide resolved
tiling.js Show resolved Hide resolved
tiling.js Show resolved Hide resolved
topbar.js Outdated Show resolved Hide resolved
jtaala and others added 2 commits March 8, 2023 08:13
Co-authored-by: Matthias Seiffert <matthias-seiffert@hotmail.de>
@Lythenas
Copy link
Collaborator

Lythenas commented Mar 8, 2023

On my work computer right now (running Gnome 42.5, if that matters) and I'm currently using dash-to-panel (don't remember exactly why that extension, probably just so I have the topbar on all monitors). In general it seems to work. But two small issues:

  • The "Show Applications" button is misaligned. But only on the primary monitor. And only after switching focus to secondary monitor and back. (could be cause by dash-to-panel)
  • The workspace name is now shown twice on the primary monitor and the name is now also shown on the secondary monitor (which I like) but is badly aligned.
  • Also when switching workspaces (Super + PageUp/Down) the workspace name is also misaligned (similar to the second monitor) and there is a gray bar at the top (maybe the backdrop).

image

image

Regarding dash-to-panel:

I also had to manually set it transparent in its settings. But that is probably fine.

As far as I know gnome-shell has no generic topbar API. I'm not sure we should specifically support dash-to-panel. But I think we should either:

  1. provide a way to show the topbar on all monitors (maybe that is now also possible with vanilla gnome-shell)
  2. support at least one extension that does that.

I've also been working on a PR that would show the "Workspace X" menu on all topbars and I didn't find a way to generically get all topbar. I.e. I had to access global.dashToPanel which is set by that specific extension.

I don't have any particular reason to use dash-to-panel over any other extension. So I'm happy to switch to another. But we can also discuss this later.

…(including `Make topbar follow monitor

focus` option).
@jtaala
Copy link
Collaborator Author

jtaala commented Mar 8, 2023

NOTE: I'm still working through some bugs with the Make topbar follow monitor focus option... grrrr ✔️

Added fix/final space layout call to initWorkspaces to allow for topbar/window position
bar init completion.
return topbar to primary and then set monitor.
@Lythenas
Copy link
Collaborator

Lythenas commented Mar 8, 2023

If I have a fullscreen application (e.g. a fullscreen youtube video, or just firefox with F11) the window indicator is not shown (or I guess it is below the window), which is good. But if I then focus a different monitor. The window indicator is shown above the fullscreen window. If I focus the fullscreen window again, it corrects itself (after a short delay). This also happens if show-window-position-bar is disabled (but just with the backdrop).

Also with your multi monitor changes the backdrop is also shown if I disable show-window-position-bar. I think we should (as far as possible) keep the old behavior if the feature is disabled.

And it seems like disabling the feature does not return the original opacity of the topbar.

With dash-to-panel there are still some issues (actually even more after the multi monitor changes^^). But I will try to think of something independent of this PR.

@Lythenas
Copy link
Collaborator

Lythenas commented Mar 8, 2023

Also I think we should decide if this will now be the new default behavior or if it will just be an addon feature.

In any case I think we should also add a setting for this in the UI. But that can be a separate PR when we know there are no more bugs.

@Lythenas
Copy link
Collaborator

Lythenas commented Mar 8, 2023

Not sure if this was broken by this PR, but the workspace border/header when navigating is different that it used to be:

Now:
Screencast from 2023-03-08 20-57-53.webm

Previous (from README):
https://github.com/paperwm/media/raw/master/sequence.png

@jtaala
Copy link
Collaborator Author

jtaala commented Mar 8, 2023

Not sure if this was broken by this PR, but the workspace border/header when navigating is different that it used to be:

Now: Screencast from 2023-03-08 20-57-53.webm

Previous (from README): https://github.com/paperwm/media/raw/master/sequence.png

Ah ha yes! that was by design (shows the backdrop and indicator just like normal view). My thinking:

  • I wanted to have the workspace previews (when switching) look like the actual view;
  • it's easier (for me at least) to see the workspace name, and much easier to see the "focus mode" icon in #482, regardless of what workspace colour or background you use (e.g. light colours or light background - I prefer seeing my background). I note issue Make the default workspace color palette better #127 re workspace colours and some visual issues it can bring - this choice was to avoid/minimise this. E.g. make the workspace name (and other stuff like PR #482) easy to see regardless of workspace colours);
  • you can actually see and use the workspace indicator from switching view (give it a try with switching windows with shortcut keys from the workspace switching view. It then looks the same as in normal views (albeit a zoomed out view) ;
  • looks betterer! (although, this could just be an opinion of mine);

@jtaala
Copy link
Collaborator Author

jtaala commented Mar 8, 2023

If I have a fullscreen application (e.g. a fullscreen youtube video, or just firefox with F11) the window indicator is not shown (or I guess it is below the window), which is good. But if I then focus a different monitor. The window indicator is shown above the fullscreen window. If I focus the fullscreen window again, it corrects itself (after a short delay). This also happens if show-window-position-bar is disabled (but just with the backdrop).

Good pickup! Will fix that.

Also with your multi monitor changes the backdrop is also shown if I disable show-window-position-bar. I think we should (as far as possible) keep the old behavior if the feature is disabled.

Yeah, sounds good - I originally thought to just leave the backdrop there but what you mentioned is probably a better choice (return things to how they were).

And it seems like disabling the feature does not return the original opacity of the topbar.

Original opacity - you mean like being opaque? Interestingly, PaperWM TopBar.js sets the opacity to be slightly transparent (assuming it will stay like that) but gnome can clobber it (e.g. without this PR, right after a login, try going into overview and then clicking a window - gnome will set the topbar opaque). This PR fixes that long-standing issue (although it might not be what people are used to now?).

windowPositionBar actors and restores original topbar.
@jtaala jtaala force-pushed the indication-of-viewport-position branch from 4655d57 to 7151eda Compare March 9, 2023 13:10
@jtaala
Copy link
Collaborator Author

jtaala commented Mar 9, 2023

If I have a fullscreen application (e.g. a fullscreen youtube video, or just firefox with F11) the window indicator is not shown (or I guess it is below the window), which is good. But if I then focus a different monitor. The window indicator is shown above the fullscreen window. If I focus the fullscreen window again, it corrects itself (after a short delay). This also happens if show-window-position-bar is disabled (but just with the backdrop).

Fixed this.

@jtaala
Copy link
Collaborator Author

jtaala commented Mar 9, 2023

Also with your multi monitor changes the backdrop is also shown if I disable show-window-position-bar. I think we should (as far as possible) keep the old behavior if the feature is disabled.

Have implemented this. Now, disabling the window position bar will completely disable it and revert back to previous behaviour.

Although, the issue with the topbar opacity (starting out as transparent but then gnome cloberring it to opaque) has been fixed (as in, PaperWM will enforce that transparency as it was originally designed to). I'm remiss to revert back to the broken behaviour - if we want the topbar to be opaque like vanilla gnome then we should just remove that transparency code. I do note that #344 suggests NOT having PaperWM change transparency.

Now, we use transparency to good effect with the window position bar (and I like it! lol) but, I'm open to suggestions re this, for example we could:

  1. keep transparency and fix the code (current behaviour with this PR);
  2. do away with transparent topbar and let users add that if wanted (via css styling in PaperWM or other extensions etc.);
  3. hybrid approach: if window position bar is disabled then we could have an opaque topbar (vanilla gnome) and keep transparency for when window position bar is enabled;

Option 2 would simplify things I guess. If not, will go ahead with the first option (since that's what we've got atm).

@jtaala
Copy link
Collaborator Author

jtaala commented Mar 9, 2023

@Lythenas, fixed those issues you raised, could you give the latest a test when you can? Cheers!

Copy link
Collaborator

@Lythenas Lythenas left a comment

Choose a reason for hiding this comment

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

This looks good to me. I haven't noticed any more issues.

Thanks for the awesome feature.

@jtaala jtaala force-pushed the indication-of-viewport-position branch from 61700ea to b25473f Compare March 11, 2023 00:39
@jtaala jtaala force-pushed the indication-of-viewport-position branch from b25473f to 5df2228 Compare March 11, 2023 00:41
@jtaala
Copy link
Collaborator Author

jtaala commented Mar 11, 2023

This looks good to me. I haven't noticed any more issues.

Thanks for the awesome feature.

Thanks @Lythenas! I've just cleaned up some stuff and also I did end up adding the enable/disable toggle to the settings ui. I believe this feature is a big enough, prominent enough, and changes enough of the previous behaviour to justify adding it to the settings ui.

I also updated the README.md and added the video from this PR to it.

I'll merge this shortly.

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.

Show an indication of viewport position when there are offscreen windows
2 participants