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

Final changes for upcoming release #198

Merged
merged 7 commits into from
May 9, 2024
Merged

Final changes for upcoming release #198

merged 7 commits into from
May 9, 2024

Conversation

ifl0w
Copy link
Owner

@ifl0w ifl0w commented Mar 26, 2024

No description provided.

@ifl0w
Copy link
Owner Author

ifl0w commented Apr 4, 2024

@Lucki can you double-check these commits here before I create the release? :)

Copy link
Contributor

@Lucki Lucki left a comment

Choose a reason for hiding this comment

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

I haven't tried it yet. I fear that after a new login the zoom mode will change - I report later on that.

Edit: Yes, as I suspected, it doesn't work.


Other than that, there's the now duplicated reset function. Do we need that extra safety, is the function provided by GJS not enough?

And other than that that: 👍

src/manager/wallpaperManager.ts Outdated Show resolved Hide resolved
src/settings.ts Outdated Show resolved Hide resolved
types/misc/index.d.ts Outdated Show resolved Hide resolved
src/wallpaperController.ts Outdated Show resolved Hide resolved
src/prefs.ts Outdated Show resolved Hide resolved
@ifl0w
Copy link
Owner Author

ifl0w commented Apr 7, 2024

Sorry, this doesn't work. This resets the background zoom option always back to zoom on logout/reboot/suspend.

Hm, right. I didn't think about that. 🤦‍♂️

With these insights, I gravitate back to the idea of directly hooking into the system settings instead of having a copy in the extension settings. In the end, Gnome extensions aren't really external programs but rather mutations of the DE, so it is somewhat reasonable to alter the settings directly. We already do this anyway with the wallpaper path itself.

Advantages:

  • No mapping between the two settings is required, thus less room for error.
  • Easier to display and change the current state in the extension settings.

Disadvantage:

  • The spanned state will be displayed in the settings when it is activated by the manager (could be seen as an advantage in terms of transparency)

@Lucki
Copy link
Contributor

Lucki commented Apr 9, 2024

I also see this disadvantage:

  • Turning off multi-monitor requires setting two steps: Turning the option itself off and also setting the zoom mode to something different from spanned. This could be done automatically, but then we're defining a default again, which might be insufficient. I don't know how often that happens though.

@ifl0w
Copy link
Owner Author

ifl0w commented Apr 17, 2024

Turning off multi-monitor requires setting two steps: Turning the option itself off and also setting the zoom mode to something different from spanned. This could be done automatically, but then we're defining a default again, which might be insufficient. I don't know how often that happens though.

Hmm, you are right, but I think this is a reasonable trade-off. I'll give it a try and test if it works for us. If not, I'll just revert my problematic changes.

@ifl0w
Copy link
Owner Author

ifl0w commented May 5, 2024

@Lucki I finally got around to giving this another quick try. If you still see some fundamental issues, then I'll just revert my stuff for creating a release, but I think this should be reasonable now. 😅
Can you give it a try with your multi-monitor setup?

Also, I renamed the "Zoom Mode" to "Scaling Mode" as I think this is more fitting.

@Lucki
Copy link
Contributor

Lucki commented May 6, 2024

Gave it a quick test and it seems to work.

ifl0w added 7 commits May 9, 2024 13:40
This change is necessary for updating wallpapers to work on clean
installs. The number 2 now maps to zoom.

Unfortunately, the number has no direct relation to the valid values and
changing the filter on the available picture-options will cause a
regression here.
This is solely done for user feedback when changing the value.
The recommended "add_child" call doesn't behave exactly the same as
"add_actor" in 45 and causes the history section to render incorrectly.
With this commit, the working function is used depending on the major
version.
* Change the name to "Scaling Mode" to be more generic
* Bind settings UI directly to the gnome shell background and
  screen-saver settings to provide direct feedback when changing the
  value
* The internal copy of the currently selected mode is kept as string to
  restore the selected value when not using an external wallpaper
  manager with multi-monitor support
@ifl0w ifl0w merged commit ec148f1 into develop May 9, 2024
4 checks passed
@ifl0w ifl0w deleted the prepare_release branch May 9, 2024 11:53
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.

2 participants