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

Redesign decoration to LibAdwaita look #145

Closed
wants to merge 1 commit into from
Closed

Conversation

urFate
Copy link

@urFate urFate commented Jul 12, 2023

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

@grulja
Copy link
Collaborator

grulja commented Jul 13, 2023

Hi, thanks for opening it again. Before I dive into the proper full review, few comments:

  1. Can you squash both commits into one?
  2. There seem to be unrelated coding-style changes, like change in the copyright header where you remove some spaces

@urFate
Copy link
Author

urFate commented Jul 13, 2023

Hi, thanks for opening it again. Before I dive into the proper full review, few comments:

  1. Can you squash both commits into one?
  2. There seem to be unrelated coding-style changes, like change in the copyright header where you remove some spaces

Yes, sorry again, didn't saw that.

@grulja
Copy link
Collaborator

grulja commented Jul 13, 2023

There are still unrelated coding style changes. I recommend running clang-format locally.

@urFate
Copy link
Author

urFate commented Jul 13, 2023

There are still unrelated coding style changes. I recommend running clang-format locally.

It should be fine now.

@grulja
Copy link
Collaborator

grulja commented Jul 21, 2023

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.

@grulja
Copy link
Collaborator

grulja commented Jul 25, 2023

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.

@urFate
Copy link
Author

urFate commented Jul 25, 2023

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.

@grulja
Copy link
Collaborator

grulja commented Jul 25, 2023

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)
@urFate
Copy link
Author

urFate commented Jul 30, 2023

Shadows has been restored.

@grulja
Copy link
Collaborator

grulja commented Aug 8, 2023

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 libdecor but that's currently a no-go for QtWayland so I gave up on that. Instead, I'm writing a completely new decoration plugin, basically taking parts of QGnomePlatform and Adwaita-qt, but avoiding GTK and Adwaita-qt as dependencies so the plugin can eventually be upstreamed to Qt. I changed the buttons to make them look like GTK4 version and drastically simplified the code. Currently the only missing part is to reimplement shadows and get correct colors from GTK4. I might have a fully working version by the end of this week.

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.

@grulja
Copy link
Collaborator

grulja commented Aug 8, 2023

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 QT_WAYLAND_DECORATION=adwaita to use them.

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.

@urFate
Copy link
Author

urFate commented Aug 9, 2023

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 libdecor but that's currently a no-go for QtWayland so I gave up on that. Instead, I'm writing a completely new decoration plugin, basically taking parts of QGnomePlatform and Adwaita-qt, but avoiding GTK and Adwaita-qt as dependencies so the plugin can eventually be upstreamed to Qt. I changed the buttons to make them look like GTK4 version and drastically simplified the code. Currently the only missing part is to reimplement shadows and get correct colors from GTK4. I might have a fully working version by the end of this week.

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.

Yep, I was just used eyedropper to get the colors of window decoration.

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 QT_WAYLAND_DECORATION=adwaita to use them.

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.

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.

@grulja
Copy link
Collaborator

grulja commented Aug 9, 2023

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 libdecor but that's currently a no-go for QtWayland so I gave up on that. Instead, I'm writing a completely new decoration plugin, basically taking parts of QGnomePlatform and Adwaita-qt, but avoiding GTK and Adwaita-qt as dependencies so the plugin can eventually be upstreamed to Qt. I changed the buttons to make them look like GTK4 version and drastically simplified the code. Currently the only missing part is to reimplement shadows and get correct colors from GTK4. I might have a fully working version by the end of this week.
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.

Yep, I was just used eyedropper to get the colors of window decoration.

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 QT_WAYLAND_DECORATION=adwaita to use them.
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.

As I understand it, you decided to rewrite the scenery plugin from scratch.

It's not completely from scratch, I just took decorations from here and simplified many things and dropped dependendencies on Adwaita-qt and Gtk.

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.

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.

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.

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.

@grulja
Copy link
Collaborator

grulja commented Aug 11, 2023

QGnomePlatform is now unmaintained and not actively developed. For decoration improvements please submit your change to https://github.com/FedoraQt/QAdwaitaDecoration instead.

@grulja grulja closed this Aug 11, 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.

2 participants