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

Cleanup settings in override #138

Closed

Conversation

worldofpeace
Copy link
Contributor

Breakdown of dropped settings:

  • auto-maximize=false
    I believe its been discussed in the gala issue tracker
    that it's desired for almost fullscreen windows to
    be auto maximized. And notoriously I've noticed
    this with applications like epiphany, where it's allmost
    fullscreen, and it makes the panel look very strange
    not being completely opaque and having a gap.
    I haven't noticed issues with it being set.

Introduced in de894b7.

  • org.gnome-settings-daemon.plugins.cursor
    This was introduced to workaround a bug where
    cursors would be invisible with g-s-d. The report 0
    was never actually closed, and I haven't noticed any
    one's recently. I believe the issue is now gone in recent versions.
    ( reports mentioned gtk 3.10)

  • org.gnome.settings-daemon.plugins.power

  • ambient-enabled=false

  • idle-dim=false

These are just powersettings that are defaults
in g-s-d that appear to be completely sane.


I've been using this in NixOS and Soon™ a downstream patch like the rest of the PRs in this repo...

Breakdown of dropped settings:

- auto-maximize=false
I believe its been discussed in the gala issue tracker
that it's desired for almost fullscreen windows to
be auto maximized. And notoriously I've noticed
this with applications like epiphany, where it's allmost
fullscreen, and it makes the panel look very strange
not being completely opaque and having a gap.
I haven't noticed issues with it being set.

Introduced in de894b7.

- org.gnome-settings-daemon.plugins.cursor
This was introduced to workaround a bug where
cursors would be invisible with g-s-d. The report [0]
was never actually closed, and I haven't noticed any
one's recently. I believe the issue is now gone in recent versions.
( reports mentioned gtk 3.10)

- org.gnome.settings-daemon.plugins.power
- ambient-enabled=false
- idle-dim=false

These are just powersettings that are defaults
in g-s-d that appear to be completely sane.

[0]: https://bugzilla.gnome.org/show_bug.cgi?id=694758
@worldofpeace worldofpeace changed the title Cleanup settings in override. Cleanup settings in override Oct 5, 2019
[org.gnome.settings-daemon.plugins.media-keys]
screensaver='<Super>l'
terminal='<Super>t'

[org.gnome.settings-daemon.plugins.power]
ambient-enabled=false
idle-dim=false
Copy link
Contributor

Choose a reason for hiding this comment

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

This is intentional. Instead of dimming the display on idle, we turn the display off; it felt weird to have both settings here and there's no visual way to toggle this one. I would be open to a counter-argument and more input from @elementary/ux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't recall if the idle-dim is happening at a smaller interval and then the display is turned off.
(dimming away)

I know at least, in Pantheon, the transition appears abrupt.
I'd have to investigate the default behavior and how it works in g-s-d though.

[org.gnome.settings-daemon.plugins.media-keys]
screensaver='<Super>l'
terminal='<Super>t'

[org.gnome.settings-daemon.plugins.power]
ambient-enabled=false
Copy link
Contributor

Choose a reason for hiding this comment

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

I forget why this was disabled; I think because it seems dishonest to tell people it's on when it's unclear if it's actually supported? If we can actually detect if it's supported, that would be better. @danrabbit do you remember?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cassidyjames It's likely there's no ui for this and g-s-d just disables it when appropriate.

Copy link
Contributor

@cassidyjames cassidyjames Oct 5, 2019

Choose a reason for hiding this comment

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

@worldofpeace there is a UI; check System SettingsPower on a laptop, or the equivalent panel in GNOME. The problem may have also been that it's a poor experience by default; I think @danrabbit had made this call originally so I'm going to defer to him. I also do not have hardware with an ambient light sensor, so I can't test it.

Copy link
Member

Choose a reason for hiding this comment

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

I think at the time we disabled it, support was really bad. It could be better now. It's worth re-evaluating

@@ -136,18 +135,10 @@ active=false
[org.gnome.settings-daemon.plugins.color]
night-light-temperature=4500

[org.gnome.settings-daemon.plugins.cursor]
# Workaround upstream gnome-settings-daemon bug (https://bugzilla.gnome.org/show_bug.cgi?id=694758) as tracked by elementary (https://bugs.launchpad.net/elementaryos/+bug/1248747)
active=false
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh, this we can probably drop. I wonder if it has anything to do with the cursor size stuff, too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tested this and it does seem to improve the cursor being the same size, and it looks like it also fixes support for the cursor size setting.

elementary/gala#300

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

auto-maximize is intentionally false. The others are open to discussion.

@cassidyjames
Copy link
Contributor

This also makes me think: we should probably have a comment with why on all overrides. There are reasons behind each but sometimes it's hard to remember.

@worldofpeace
Copy link
Contributor Author

auto-maximize is intentionally false. The others are open to discussion.

Thanks, though how have you closed the discussion on auto-maximize? Is there some precedent in your tracker that I'm not aware off? I'd be more open to dropping this change if it didn't appear you decided this for me.

@worldofpeace
Copy link
Contributor Author

This also makes me think: we should probably have a comment with why on all overrides. There are reasons behind each but sometimes it's hard to remember.

Very much so, git-blame here wasn't helpful for these types of things.

@cassidyjames
Copy link
Contributor

@worldofpeace

I'd be more open to dropping this change if it didn't appear you decided this for me.

Well, you can do whatever you want downstream but we've decided this a long time ago for elementary OS. I've expanded on my code comment above, as well. I'm not sure where the original discussion took place (probably on IRC back when we used Launchpad, honestly), but this is still very intentionally the design of how Pantheon works.

@worldofpeace
Copy link
Contributor Author

@worldofpeace

I'd be more open to dropping this change if it didn't appear you decided this for me.

Well, you can do whatever you want downstream but we've decided this a long time ago for elementary OS. I've expanded on my code comment above, as well. I'm not sure where the original discussion took place (probably on IRC back when we used Launchpad, honestly), but this is still very intentionally the design of how Pantheon works.

Thanks, I think this feeds into the point that default settings in elementary OS lack documentation.
And I prefer not to change defaults like this downstream if possible, especially when it it's a design decision you've made for the DE. It could reflect poorly on Pantheon and give confusion as to why there's differing behaviors. Usually only needed when there's a consistent user feedback something is degrading user experience for us specifically. And even then trying to avoid fragmentation.

@cassidyjames
Copy link
Contributor

@worldofpeace as annoying as it might be… could you propose each of these as a PR? That way each can be discussed/tested/merged on their own without holding the others up.

@worldofpeace
Copy link
Contributor Author

@worldofpeace as annoying as it might be… could you propose each of these as a PR? That way each can be discussed/tested/merged on their own without holding the others up.

Possibly. Would multiple commits be acceptable? I don't think much of the changes here are controversial, so testing a PR commit by commit could be similar.

@cassidyjames
Copy link
Contributor

Eh, multiple PRs fits in with the GitHub and elementary workflow better and makes it more likely at least some of these changes will be accepted more quickly.

@worldofpeace
Copy link
Contributor Author

Eh, multiple PRs fits in with the GitHub and elementary workflow better and makes it more likely at least some of these changes will be accepted more quickly.

Will do, there's an "elementary" workflow?
(semantically I'm expecting it to value simplicity 😄 )

@cassidyjames
Copy link
Contributor

Yes, one change per PR for ease of testing and merging on its own merits. ;)

@worldofpeace
Copy link
Contributor Author

Superseded by
#139
#140
#141

I rescinded the auto-maximize change.

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.

None yet

3 participants