GNOME Bugzilla – Bug 772546
Bring back Unity-specific workarounds
Last modified: 2017-10-11 05:45:41 UTC
Unity-specific workarounds was removed in 7b37e26 (bug 738899). The reason given was that Unity,now,is capable of supporting gtk-headerbar. But even though it has support (barely) for it, it doesn't blend well under Unity or any other compiz session (ex: flashback-compiz, mate-compiz) due to these bugs: 1. https://bugs.launchpad.net/ubuntu/+source/compiz/+bug/1478272 2. https://bugs.launchpad.net/ubuntu/+source/unity/+bug/1570843 3. https://bugs.launchpad.net/ubuntu/+source/unity/+bug/1387163 As a result it makes things quite cumbersome to use headerbar app under these compiz sessions. The workarounds basically does two major things: 1. Geary uses traditional titlebar instead of CSD for both main window and detached composer window. 2. It depends on Unity's indicator-datetime to get the user preference of time format (12 hour or 24 hour). It would be nice if we have these workarounds in geary so that it could just work on Ubuntu, as has always been the case in the past.
I'll be happy to take/make patches for this which do the following: 1. Like 7b37e26, if Unity is detected makes Geary display a titlebar. 2. Use the message indicator pref for date/time, but for the message indicator only 3. Unlike 7b37e26, doesn't re-introduce the use of the GearyApplication.instance static var.
Hi, I would like to contribute bringing workarounds back. I would introduce new switch in Configuration called 'display_titlebar'. This switch would be currently set to true only in case we detect Unity. I have a few questions however: > 2. Use the message indicator pref for date/time, but for the message indicator only I do not get what exactly was wrong in the previous implementation of 'clock_format' method that was removed in 7b37e26? > 3. Unlike 7b37e26, doesn't re-introduce the use of the GearyApplication.instance static var. I see the usage of GearyApplication.instance usage across many classes. What is preferred way of accessing Configuration object? I see that currently it is done by calling GearyApplication.instance.config.
Hi Kacper, Thanks for looking into this. (In reply to Kacper Bielecki from comment #2) > I would introduce new switch in Configuration called 'display_titlebar'. > This switch would be currently set to true only in case we detect Unity. Using the config object sounds good, however we will likely need to do something similar for other desktops such as XFCE and KDE, and they may have their own quirks. So rather than populating the Configuration object with a property for every quirk that every desktop has, I'd prefer to see the config object returning the current detected desktop as a value from a "DesktopEnvironment" enum or something. The app can then modify its behaviour to do things like use CSD or not, depending on the DE in use. For the moment, the enum would only need to have UNKNOWN and UNITY. > I do not get what exactly was wrong in the previous implementation of > 'clock_format' method that was removed in 7b37e26? It was using an indicator-specific setting for an app-wide setting - so it was more of a problem of the scope of where it was used rather than how it was implemented. It seems that Ubuntu intends this to be used only for the desktop clock and indicators, since the UI for this pref is disabled if the user also disables the desktop clock. So if it needs to be brought back, then adding it back to Geary's indicator implementation would be the right way to go, rather than making it app-wide. > What is preferred way of accessing Configuration object? I see that > currently it is done by calling GearyApplication.instance.config. That will depend on the situation, but usually the best way to do it would be in GearyController, pass the config object as a constructor argument to objects that need access to it.
Thanks! I am working on it. I should have patch ready by next week.
Created attachment 339303 [details] [review] Bring back titlebar on Unity This patch brings back titleabar display in Unity. I couldn't find any usage of date/time in new message notification so I didn't bring this functionality back. Let me know if I am wrong. I tested it on Unity with Ubuntu 16.10. Geary main window and new message window look fine. On unity: - App menu is displayed in top screen menu bar with "Show the menus for a window" set to "In the menu bar" - App menu is displayed in top window bar with the same setting set to "In the windows's title bar"
What about date-time format in message header? For example if I set 12 hr format in Unity's date-time preference I want to see message header in same format. (see screenshot: http://i.imgur.com/foFLvw1.png) Where does geary (master) get that date-time preference? In Unity, I don't know if there is any other way to set it globally other than clock preference).
I agree with Michael that clock format setting in Unity clock indicator prefs should apply only to clock indicator. Geary takes clock settings from "org.gnome.desktop.interface" clock-format setting. This setting is also present on Ubuntu Unity. However, it is not affected neither by: - clock indicator setting - Unity language settings - Gnome language settings I can't find any way to change "org.gnome.desktop.interface" clock-format setting using UI neither on Gnome 3.20 nor on Unity on ubuntu 16.10. This is why I believe that using "org.gnome.desktop.interface" clock-format setting by Geary is buggy. Geary should simply format date and time using locale specific time format string: - %X - the preferred time representation for the current locale without the date - %c - the preferred date and time representation for the current locale What do you think about this? If you agree I could implement it in a separate Bug as this is not Unity specific any more.
(In reply to Khurshid Alam from comment #6) > What about date-time format in message header? For example if I set 12 > hr format in Unity's date-time preference I want to see message header > in same format. (see screenshot: http://i.imgur.com/foFLvw1.png) Same way you set it for Nautilus or Firefox, or the GTK+ file chooser? If Ubuntu is patching GTK+ and apps like these to use the indicator setting, then I'm happy to also do the same in Geary. If not, then I'd argue that consistency with the rest of the desktop is more important. (In reply to Kacper Bielecki from comment #7) > I can't find any way to change "org.gnome.desktop.interface" clock-format > setting using UI neither on Gnome 3.20 nor on Unity on ubuntu 16.10. Under GNOME, you can change it using the Date & Time control panel: the Time Format pref at the bottom. Note that changing this also changes the org.gtk.Settings.FileChooser clock-format setting to stay in sync. > This is why I believe that using "org.gnome.desktop.interface" clock-format > setting by Geary is buggy. Geary should simply format date and time using > locale specific time format string: I would agree with you, since the org.gnome.desktop.interface setting seems like it should have the same intent as the indicator setting under Ubuntu, except that changing the setting in gnome-control-centre also changes the format in Nautilus, the GtkFileChooser, and elsewhere - hence the intent is actually to apply it desktop-wide, and not just to the gnome-shell clock. > What do you think about this? If you agree I could implement it in a > separate Bug as this is not Unity specific any more. It's definitely not something to hash out in this bug, but I'd suggest that the actual problem is that 1) GNOME's clock-setting value doesn't have a "use-locale" option, and 2) that it should probably be available in GTK+ as a GSetting anyway, so all GTK apps can just query a single place to work out what format to use. Feel free to open a bug against Geary for this as well, for the record.
Review of attachment 339303 [details] [review]: Hey Kacper, thanks for the updated patch. This looks pretty good overall. Just one style and one question about Ubuntu's practices below. I haven't checked it for any trailing whitespace introduced, either. Also, as I mentioned just before above, if Ubuntu patches other GTK+ apps such as Nautilus to use the indicator pref for setting the time format instead of the GNOME pref, then I'm happy to re-include that here as well. ::: src/client/components/main-window.vala @@ +22,3 @@ public StatusBar status_bar { get; private set; default = new StatusBar(); } public Geary.Folder? current_folder { get; private set; default = null; } + public GearyApplication geary_application { get; private set; } Since the geary_application property is only used to obtain an instance of the config object, I'd suggest either: - Replacing it with a property of type Configuration - Override Gtk.ApplicationWindow's application property to return an object of type GearyApplication (assuming GLib/vala allows this) ::: src/client/components/pill-toolbar.vala @@ +63,3 @@ + if (config.desktop_environment == Configuration.DesktopEnvironment.UNITY) { + b.image.margin += 4; + } Is something that Ubuntu patches other GTK+ apps to do? If not, I'd rather not mess with their theme like this.
(In reply to Michael Gratton from comment #8) > (In reply to Khurshid Alam from comment #6) > > What about date-time format in message header? For example if I set 12 > > hr format in Unity's date-time preference I want to see message header > > in same format. (see screenshot: http://i.imgur.com/foFLvw1.png) > > Same way you set it for Nautilus or Firefox, or the GTK+ file chooser? If > Ubuntu is patching GTK+ and apps like these to use the indicator setting, > then I'm happy to also do the same in Geary. If not, then I'd argue that > consistency with the rest of the desktop is more important. I have no idea about Firefox. But I checked GTK+ file chooser and Nautilus. - Gtk+ file chooser is not changing clock format [1] according to Unity indicator setting - nautilus also does not change time format according to Unity indicator setting. Instead it relies on gnome setting in some places and on locale in others [2] It also looks like Unity knows about the problem with Gnome apps relying on Gnome clock setting [3], however it is not very high on priority list. > > (In reply to Kacper Bielecki from comment #7) > > I can't find any way to change "org.gnome.desktop.interface" clock-format > > setting using UI neither on Gnome 3.20 nor on Unity on ubuntu 16.10. > > Under GNOME, you can change it using the Date & Time control panel: the Time > Format pref at the bottom. Note that changing this also changes the > org.gtk.Settings.FileChooser clock-format setting to stay in sync. I can't find this setting in Gnome 3.22 any more [4]. > > > This is why I believe that using "org.gnome.desktop.interface" clock-format > > setting by Geary is buggy. Geary should simply format date and time using > > locale specific time format string: > > I would agree with you, since the org.gnome.desktop.interface setting seems > like it should have the same intent as the indicator setting under Ubuntu, > except that changing the setting in gnome-control-centre also changes the > format in Nautilus, the GtkFileChooser, and elsewhere - hence the intent is > actually to apply it desktop-wide, and not just to the gnome-shell clock. > > > What do you think about this? If you agree I could implement it in a > > separate Bug as this is not Unity specific any more. > > It's definitely not something to hash out in this bug, but I'd suggest that > the actual problem is that 1) GNOME's clock-setting value doesn't have a > "use-locale" option, and 2) that it should probably be available in GTK+ as > a GSetting anyway, so all GTK apps can just query a single place to work out > what format to use. Feel free to open a bug against Geary for this as well, > for the record. [1] https://dl.dropboxusercontent.com/1/view/wzhduf686s4rmgf/Apps/Shutter/Auswahl_011.png [2] https://github.com/GNOME/nautilus/blob/master/src/nautilus-file.c#L5605-5606 [3] https://bugs.launchpad.net/ubuntu/+source/indicator-datetime/+bug/1174261 [4] https://dl.dropboxusercontent.com/1/view/wop9qqm8iskj9id/Apps/Shutter/date-time-setting-322.png
Created attachment 340359 [details] [review] This commit reverts Unity workaround removal (commit 7b37e26; bug 738899) I couldn't find anything specific to Unity in Nautilus or GTK codebase. Therefore, this patch do not contain visual fixes as compared with the previous patch.
Review of attachment 340359 [details] [review]: Thanks for the updated patch, and rebasing it off of current master. It's nearly good to go. I was just testing this out under Unity on Yakkety then, and it all seems to work fine, with the exception of the composer's Detach button being on the wrong side of its header bar, when it's contained within the main window. It should be on the left in a LTR desktop session, but it is appearing on the right instead (and obviously vice-versa for an RTL session). If you can address this and the two trivial points below, then it will be good to commit. ::: src/client/components/main-toolbar.vala @@ +21,3 @@ + private Configuration config { get; private set; } + Is this class member needed if the config object is only used within the constructor? ::: src/client/composer/composer-window.vala @@ +24,1 @@ + public ComposerWindow(ComposerWidget composer, Configuration config) { Instead of passing in the config argument here, you could probably just access it via the ComposerWidget.config property.
(In reply to Kacper Bielecki from comment #10) > > I have no idea about Firefox. But I checked GTK+ file chooser and Nautilus. > > - Gtk+ file chooser is not changing clock format [1] according to Unity > indicator setting > - nautilus also does not change time format according to Unity indicator > setting. Instead it relies on gnome setting in some places and on locale in > others [2] > > It also looks like Unity knows about the problem with Gnome apps relying on > Gnome clock setting [3], however it is not very high on priority list. Okay, well I'd prefer to keep it consistent with those apps for now, but that's not a problem with the patch. We can look at revisiting that when Unity fix lp:1174261 > > Under GNOME, you can change it using the Date & Time control panel: the Time > > Format pref at the bottom. Note that changing this also changes the > > org.gtk.Settings.FileChooser clock-format setting to stay in sync. > > I can't find this setting in Gnome 3.22 any more [4]. It's definitely there - defined as a GSettings in org.gtk.Settings.FileChooser.gschema.xml. Under dconf it is found under: "/org/gtk/settings/file-chooser/clock-format".
(In reply to Michael Gratton from comment #12) > Review of attachment 340359 [details] [review] [review]: > I was just testing this out under Unity on Yakkety then, and it all seems to > work fine, with the exception of the composer's Detach button being on the > wrong side of its header bar, when it's contained within the main window. It > should be on the left in a LTR desktop session, but it is appearing on the > right instead (and obviously vice-versa for an RTL session). Position of detach button depends in Geary on gtk-decoration-layout property [1], [2]. Unfortunately, Unity seems to not set this property which for which I wrote workaround in 770617. Do you think it is fine it will be always on the right in Unity? [1] https://github.com/kazjote/geary/blob/738899-unity-back-rebase/src/client/util/util-gtk.vala#L127 [2] https://github.com/kazjote/geary/blob/738899-unity-back-rebase/src/client/composer/composer-headerbar.vala#L63
(In reply to Kacper Bielecki from comment #14) > > Position of detach button depends in Geary on gtk-decoration-layout property > [1], [2]. Unfortunately, Unity seems to not set this property which for > which I wrote workaround in 770617. > > Do you think it is fine it will be always on the right in Unity? Oh okay, yeah normally we position it according to the user's CSD prefs. Since Unity does not offer any control over where the WM buttons go, then yes I agree we can always assume the button should be packed at the end — i.e. on the right in LTR sessions.
Created attachment 340710 [details] [review] Detach composer button always right on Unity
Review of attachment 340710 [details] [review]: Pushed to master as commit 01c419a. Thanks heaps!
Okay, so I think the last thing to do here overlaps with Bug 770617 - what to do about about the app menu. Probably best to take that over there, so I'll close this as fixed for now.
It's great to see this land! Kacper and Michael, thanks for your cooperation in making this happen!
This patch needs to be updated for recent Ubuntu version where xdg_current_desktop name has changed. Unity for Ubuntu < 16.04 Unity:Unity7 for 17.04 Unity:Unity7:ubuntu >= 17.10
(In reply to Khurshid Alam from comment #20) > Unity:Unity7:ubuntu >= 17.10 I have just installed Ubuntu 17.10 beta2 from http://de.releases.ubuntu.com/17.10/ubuntu-17.10-beta2-desktop-amd64.iso and I see that XDG_CURRENT_DESKTOP environment variable has "ubuntu:GNOME" value. @Khurshid: where did you get "Unity:Unity7:ubuntu" value from? Maybe it has changed recently or is going to change in the final edition?
@Kacper "ubuntu:GNOME" is for default gnome-shell session in 17.10. However Unity7 is still maintained and will be shipped along with shell in the final release. But you can also install it in beta using: sudo apt-get install unity-session Then logout, choose Unity from gdm, and login into unity. Note: If you have trouble logging into unity-session from gdm, install lightdm, make it default display manager, reboot and then log-in into unity-session.
Created attachment 361203 [details] [review] Update Unity specific workarounds Workarounds work now correctly with Ubuntu >= 16.10.
Thank you @Khurshid I submitted a new patch. I tested it on: - Ubuntu 16.10 - Ubuntu 17.10 with Unity installed It works correctly there.
Review of attachment 361203 [details] [review]: Looks good, thanks for the update. Pushed as commit dc6fdeb to master and geary-0.12 as commit c7f0061.
Thank you. @Michael and @Kacper.