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

Button placement key #996

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

orowith2os
Copy link

@orowith2os orowith2os commented Mar 29, 2023

Closes #956

This only handles the layout of window decorations, the window decorations visuals themselves would probably best be handled separately, if at all.

Client implementations:

  • GTK
  • QT
  • Electron + ArmCord
  • Firefox

Portal implementations:

@orowith2os
Copy link
Author

I'm not entirely sure why workflows are failing, help would be appreciated there.

@orowith2os
Copy link
Author

I made https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/200 to add the appropriate mode to xdg-decoration.

@orowith2os
Copy link
Author

Update: still waiting on more input on how I should define this. The Wayland merge request is also useless here, was just a misunderstanding; server-side means apps shouldn't draw decorations at all, client-side means apps should draw them themselves. A none mode wasn't needed.

@orowith2os
Copy link
Author

Going to update and rework this just a bit, since I've learned quite a bit more about how the GTK preference works and cross-toolkit-ness.

@jadahl
Copy link
Collaborator

jadahl commented Jul 6, 2023

This seems fine to add to me, it could be useful for hide e.g. minimize where it doesn't matter etc. What I'd like to point out is that it won't solve potential "annoyances" that people have with CSD apps hiding things so they can get another "frame" placed around them. Arguably the value for this settings entry in those DE:s should still be so that CSD-only applications can, if they want, add the appropriate window controls, so that if the DE uses the "minimize" concept a lot, the app knows it should add a minimize button. Maybe this was something you already had in mind already.

@swick
Copy link
Contributor

swick commented Jul 6, 2023

The description of the setting is probably too strong. It tells clients exactly which buttons should be drawn in which order. It would probably be make more sense for it to be a hint: these actions make sense in the current environment. And then let the client figure out where to draw what buttons.

That also sidesteps issues with RTL layouts.

@orowith2os
Copy link
Author

It would probably be make more sense for it to be a hint: these actions make sense in the current environment. And then let the client figure out where to draw what buttons.

I was thinking about this when reimplementing the button-layout logic from Tweaks, and yeah, a list of ideal actions would be better. I do think telling them where they should be drawn should be done though, so that on Pantheon you can get a close button on the left and on GNOME you get a close button on the right - while having no decorations on Sway and all decorations on the right on KDE.

@orowith2os orowith2os changed the title Window decorations key Button placement key Jul 7, 2023
@orowith2os orowith2os marked this pull request as ready for review July 7, 2023 11:38
@orowith2os orowith2os requested a review from lleyton July 7, 2023 11:38
@orowith2os
Copy link
Author

Updated, and accounted for some potential configurations such that they're invalid, when they would be valid before.

Now it should work for setups such as: Sway, KDE, Pantheon, and GNOME.

@orowith2os
Copy link
Author

Docs are finally building! Glad to see it making some more progress forward from where it was last time.

data/org.freedesktop.impl.portal.Settings.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.Settings.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.Settings.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.Settings.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Settings.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@lleyton lleyton left a comment

Choose a reason for hiding this comment

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

Generally looks okay, at least from a quick look over. Personally, I'd use an int enum to represent the button types, but that's just bikeshedding from my part :p

@orowith2os
Copy link
Author

@ebassi would you mind giving this an updated review, please? I'd like to see if there's anything else to nab before I start implementing it.

@orowith2os
Copy link
Author

orowith2os commented Aug 13, 2023

Bugged @danirabbit on the Elementary Slack about it, so I think Elementary can be considered mostly neutral or in favor of this - they'd just hardcode (["close"],["maximize"]) on their side of things. Here's a portal issue for them: elementary/portals#87

Someone needs to handle it for KDE too: https://invent.kde.org/plasma/xdg-desktop-portal-kde/-/issues/14

GNOME should be more or less fine, but someone needs to first expose a ([],["close"]) on the GNOME portal, then can possibly look at converting the gsettings key to that.

GTK should be able to implement the client side of this without adding or breaking the API.

@ilya-fedin
Copy link
Contributor

btw, only GTK from client implementations mentioned in the PR seems realistic as both Firefox and Electron would rather use GtkSettings I believe and Qt has no concept of titlebar settings. Qt has only a dummy decoration for the cases when xdg-decoration is not present but then it's better to spend time on adding libdecor support I believe what ends with GTK client implementation again.

@jadahl
Copy link
Collaborator

jadahl commented Aug 24, 2023

@orowith2os don't merge main into this branch, this repository uses a linear history.

@orowith2os
Copy link
Author

Fair enough, I can force push such that there's only one commit if/when this is ready to be merged :)

@jadahl
Copy link
Collaborator

jadahl commented Aug 24, 2023

Fair enough, I can force push such that there's only one commit if/when this is ready to be merged :)

Just do so now, no reason to wait to clean up the branch.

Signed-off-by: Dallas Strouse <dastrouses@gmail.com>
Co-authored-by: Emmanuele Bassi <ebassi@gmail.com>
@orowith2os
Copy link
Author

Done.

@jadahl
Copy link
Collaborator

jadahl commented Aug 24, 2023

Next step before this can land would be to create proof of concepts and "acks" in "enough" toolkits.

@orowith2os
Copy link
Author

orowith2os commented Aug 24, 2023

I'm sure @lleyton should be able to handle the tauOS and libhelium side of things, and the work can easily be strapped onto the GNOME portal + GTK with @ItsJamie9494. I plan on making a Rust representation and reference for the GNOME portal soon, and tauOS can reuse it in their stuff.

@ItsJamie9494
Copy link

I can work with GTK and GNOME, but I have nothing to do with libhelium and/or tauOS anymore

@jadahl
Copy link
Collaborator

jadahl commented Aug 24, 2023

What about Qt (and/or https://github.com/FedoraQt/QAdwaitaDecorations)? If it's GTK land only, then it's not gaining much compared to just forwarding the GTK gsetting, does it not?

Edit: wrong link to QtADwaitaDecorations.

@jadahl
Copy link
Collaborator

jadahl commented Aug 24, 2023

Sorry, sent the wrong link to QtAdwaitaDecorations.

@orowith2os
Copy link
Author

I don't have anybody at hand to manage QT, but AFAIK that's all done with the platform plugins. You'd need to go through and update each one individually.

If I/we can sort something out for a source code representation of this setting, I can release it as a library and reuse that, possibly. Or use it as a base for other implementations.

@jadahl
Copy link
Collaborator

jadahl commented Aug 24, 2023

I don't have anybody at hand to manage QT, but AFAIK that's all done with the platform plugins. You'd need to go through and update each one individually.

That's why I linked to QtAdwaitaDecorations. See https://jgrulich.cz/2023/08/22/qt-theming-in-fedora-workstation/.

If I/we can sort something out for a source code representation of this setting, I can release it as a library and reuse that, possibly. Or use it as a base for other implementations.

"implementing" the setting is most likely 99% toolkit specific plumbing, so prototyping a library outside of a toolkit likely isn't much help - I don't imagine GTK or Qt would have any use for it.

@grulja
Copy link
Contributor

grulja commented Aug 24, 2023

In QAdwaitaDecorations I already use what's provided by org.gnome.desktop.wm.preferences (in particular button-layout key). There is not much I can gain changing that for something else since this project targets GNOME and this is expected to be provided all the time. For Qt in general, I can imagine this being part of the decoration plugin.

@orowith2os
Copy link
Author

QAdwaitaDecorations should probably still look to use this portal at some point, in the (possibly unlikely) event it's used someplace that's not GNOME, and the GNOME preferences aren't available to be read.

Comment on lines +47 to +60
<term>org.freedesktop.appearance button-placement (asas)</term>
<listitem><para>
Indicates the system's preferred arrangement of buttons on the titlebar.
In LTR locales, the first tuple is on the left (leading), and the second tuple is on the right (trailing).
In RTL locales, the first tuple is on the right (leading), and the second tuple is on the left (trailing).
Supported values are:
<simplelist>
<member>close: the close button</member>
<member>minimize: the minimize button</member>
<member>maximize: the maximize button</member>
</simplelist>
Unknown values must be ignored if the client does
not understand them. Duplicate buttons across the tuples and
string arrays are not allowed.
Copy link
Author

Choose a reason for hiding this comment

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

@ebassi @jadahl do you both have any thoughts on other paths this could take? anything nicer to expose over dbus? anything that's easier to put into code? it is Not Fun™ as-is.

@ilya-fedin
Copy link
Contributor

You'd need to go through and update each one individually.

The proper way would be to add a hint here, then implement it here and consume in decoration plugins like this (the default one is here)

    if (const QPlatformTheme *theme = QGuiApplicationPrivate::platformTheme()) {
        const QVariant themeHint = theme->themeHint(QPlatformTheme::SystemIconThemeName);
        if (themeHint.isValid())
            return themeHint.toString();
    }

The one who would like to implement this will probably have to have a talk about that at development@qt-project.org mailing list first.

@GeorgesStavracas GeorgesStavracas added new api This requires adding API to an existing portal portal: settings Settings portal needs discussion Needs discussion on how to implement or fix the corresponding task labels Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Needs discussion on how to implement or fix the corresponding task new api This requires adding API to an existing portal portal: settings Settings portal
Projects
Status: Triaged
Development

Successfully merging this pull request may close these issues.

Settings portal: WindowDecorations key
9 participants