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

Disable features that conflict with Dash to Panel #420

Closed
wants to merge 1 commit into from
Closed

Disable features that conflict with Dash to Panel #420

wants to merge 1 commit into from

Conversation

Thesola10
Copy link
Collaborator

@Thesola10 Thesola10 commented Sep 28, 2022

This allows us to detect whether Dash to Panel is enabled in the user's session, and perform the following changes:

  • Disable panel opacity control, Dash to Panel has its own
  • Disable the custom workspace button and leave Activities in place
  • Disable handler for "panel follows monitor focus"
  • Grey out "panel follows monitor focus" and add explanatory tooltip

Also added wallpaper refresh on unlock (Night Theme Switcher) and a couple fixes when locking session

This allows us to detect whether Dash to Panel is enabled in the user's
session, and perform the following changes:

- Disable panel opacity control, Dash to Panel has its own
- Disable the custom workspace button and leave Activities in place
- Disable handler for "panel follows monitor focus"
- Grey out "panel follows monitor focus" and add explanation tooltip
Comment on lines +172 to +175
if (this._settings.get_boolean('found-dash-to-panel')) {
topbarFollowFocus.set_sensitive(false);
topbarFollowFocus.get_parent().set_tooltip_text("Incompatible with Dash to Panel.");
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally I'd like to follow HIG and add an icon next to the toggle to signify that something is up

@@ -2270,7 +2275,7 @@ function registerWindow(metaWindow) {
if (metaWindow.clone) {
// Should no longer be possible, but leave a trace just to be sure
utils.warn("window already registered", metaWindow.title);
utils.print_stacktrace();
//utils.print_stacktrace();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change speeds up unlocking session drastically.

Comment on lines +374 to +378
<key type="b" name="found-dash-to-panel">
<default>false</default>
<summary>Automatically set on startup when Dash to Panel is enabled. Any changes to this setting will be reset.</summary>
</key>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently prefs.js can't access extension constants -- it exists outside of the extension manager, so I had the extension leave a message for the prefs view to find afterwards.

@Thesola10
Copy link
Collaborator Author

@smichel17 review please? It's been a while and these patches are still relevant

@smichel17
Copy link
Collaborator

Overall: I think it seems reasonable, but I am concerned with maintainability. How much compatibility code are we willing to take on (e.g. for other extensions), and what level of support do we want to maintain for it (e.g. if we want to add a new feature to PaperWM, is it okay to break extension compat or do we want to test all that first)?

Those questions make me want to look over this code and think about the architecture more closely than the skim I did a few months ago and yesterday, but I'm not sure when I'll get around to that.

Maybe a compromise in the meantime: would you be up to commit to maintaining it?

@Thesola10
Copy link
Collaborator Author

I'd say compatibility is only really a concern for extensions that conflict with PaperWM and are susceptible to be installed alongside it.

But yeah, the compat patch is relatively simple, I think I'll be able to maintain it.

by the way, have you had a look at #421 yet? It's mostly unrelated but a nice touch for convertible and tablet users.

@smichel17
Copy link
Collaborator

Life and work are a bit complicated right now, not much bandwidth. #421 is a draft so I didn't look at it.

@jtaala
Copy link
Collaborator

jtaala commented Mar 16, 2023

A few things here - looks like you're wanting to merge into gnome-42 branch? That basically means there's really no future to this feature past gnome-42 - perhaps change it to merge into develop branch.

I understand the intent here but I don't think we should be doing things specifically for any specific extension (even if it's a very cool one with dash-to-panel) - for some of the reasons @smichel17 mentioned.

Instead, we should simply make each of these options (and others) settable via dconf - and well documented. For example, we've recently released #476 - which you can disable it entirely via dconf with a:

dconf write /org/gnome/shell/extensions/paperwm/show-window-position-bar false

which you'd probably want to do if using dash-to-panel as it changes topbar opacity (so you can see the window position indicator).

We could actually add similar dconf options for opacity, showing/hide workspace or activities button etc. This makes it much more configurable rather than having external extension specific code (e.g. what if I want to use dash-to-panel but don't like the activities button and do want he workspace name button?) - having options to change just the bits users want is preferable.

@Thesola10
Copy link
Collaborator Author

@jtaala yeah, that makes a lot more sense

@Thesola10 Thesola10 closed this 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