-
Notifications
You must be signed in to change notification settings - Fork 38
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
Redesign decoration to LibAdwaita look #145
Conversation
Hi, thanks for opening it again. Before I dive into the proper full review, few comments:
|
Yes, sorry again, didn't saw that. |
There are still unrelated coding style changes. I recommend running clang-format locally. |
It should be fine now. |
Hi, going through the code, it looks good code-wise, I just didn't have opportunity to test it yet, mainly because I was a week away attending KDE Akademy conference and also because I don't use GNOME by default and running Kinoite makes it harder to install it. My proposal to remove custom Qt theming for Fedora 39 has been approved so I will start intensively working on it next week and I will finally get to test your change and compare it to the current GNOME upstream. Thank you for your patience. |
I switched now to GNOME and I can see for example with Nautilus that GTK4/libadwaita apps still have shadows around the window, while your patch removes them completely. |
Oh, really? It was not easy for me to notice them on the configuration of my monitor, so I decided that they simply did not exist. As soon as I am free, I will revert the removal of shadows in the commit. |
They are very subtle and barely visible, but they exist. I don't think they actually changed between GTK3 and GTK4. and our shadows definitely have room for improvements. |
(cherry picked from commit 280e210)
Shadows has been restored. |
Hi, just to let you know what's going on. As I mentioned, I'm planning to discontinue QGnomePlatform, but we will still need some kind of client-side decorations for Qt on GNOME. I started looking into May I ask where did you get the colors? Did you take them from the GTK4 stylesheet? Or did you just took a picture and used some image editor to get them? I don't like the idea of hardcoding these colors, ideally we would have a way to get them on runtime, but that's not probably possible, especially when not having GTK as a dependency. |
Sharing what I have in case you want to look and possibly contribute your changes updating colors? The repo is now here: https://github.com/FedoraQt/QAdwaitaDecoration I have some other work planned for today and tomorrow, but I can continue with implementing the shadows later. Just saying in case you want to look into implementing the proper colors? Currently our only option is to probably hardcode these colors the same way we have here in QGnomePlatform. Btw. to build the decoration plugin you need QtWayland from Fedora (API changes) which I suppose you use? And use Edit: I see you are probably using Archlinux. The patch I'm talking about we have in qt5-qtwayland is this one. It's needed for proper shadows support. |
Yep, I was just used eyedropper to get the colors of window decoration.
As I understand it, you decided to rewrite the scenery plugin from scratch. I can help with the colors, I'll try to look for the original stylesheet and take the colors from there, although I doubt they will be different from the ones I got from the color picker. However, I still have to figure it all out a bit. Regarding the last two paragraphs, as I understand it, I need a new patch for qt5 to build this project or was it just a clarification? I actually use arch linux. |
It's not completely from scratch, I just took decorations from here and simplified many things and dropped dependendencies on Adwaita-qt and Gtk.
Taking the colors using the color picker is fine for now, but I would like to just know where these are defined so we can keep these in sync and watch for changes. I would do it myself, it's not a big task, I just thought you want to help and don't want your effort you put into this change being useless.
Yes, you need that patch for Qt5. I didn't bother with all the ifdefs we have here in QGnomePlatform as I'm targetting Fedora (where that patch is included) and when potentially upstreaming this, there won't be a Qt 5 version. |
QGnomePlatform is now unmaintained and not actively developed. For decoration improvements please submit your change to https://github.com/FedoraQt/QAdwaitaDecoration instead. |
It's me again. I had to open a new pull request because after the upstream changes, the history of previous commits was erased for some reason, I don't think it's critical.
Result
🌑 Dark Scheme
☀️ Light Scheme