After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 704086 - Screen brightness OSDs should appear on the affected screen
Screen brightness OSDs should appear on the affected screen
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: media-keys
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
: 740034 (view as bug list)
Depends on: 740074
Blocks:
 
 
Reported: 2013-07-12 12:31 UTC by Giovanni Campagna
Modified: 2014-11-13 18:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
power: pass the monitor id to OSD notifications for brightness (9.07 KB, patch)
2014-11-13 18:07 UTC, Cosimo Cecchi
needs-work Details | Review
power: add a method to retrieve the backlight output id (2.08 KB, patch)
2014-11-13 18:26 UTC, Cosimo Cecchi
reviewed Details | Review
power: return output id when changing screen brightness (4.26 KB, patch)
2014-11-13 18:26 UTC, Cosimo Cecchi
committed Details | Review
media-keys: pass output id to show_osd() (4.35 KB, patch)
2014-11-13 18:26 UTC, Cosimo Cecchi
committed Details | Review
power: add a method to retrieve the backlight output id (2.05 KB, patch)
2014-11-13 18:29 UTC, Cosimo Cecchi
committed Details | Review

Description Giovanni Campagna 2013-07-12 12:31:19 UTC
It would make more sense to me, at least
Comment 1 Florian Müllner 2014-04-26 21:09:06 UTC
Since bug 712664, it is possible to specify the monitor index the OSD should appear on. It's up to g-s-d to use it for the brightness OSD, so reassigning.
Comment 2 Bastien Nocera 2014-11-12 21:07:16 UTC
*** Bug 740034 has been marked as a duplicate of this bug. ***
Comment 3 Cosimo Cecchi 2014-11-13 18:07:30 UTC
Created attachment 290651 [details] [review]
power: pass the monitor id to OSD notifications for brightness

It only makes sense to show the brightness indicator on the display the
brightness of which is being controlled.
Pass the monitor id to the brightness OSD window so that the shell will
place it on the right monitor for us.
Comment 4 Bastien Nocera 2014-11-13 18:16:37 UTC
Review of attachment 290651 [details] [review]:

I'd prefer the power plugin changes to be separate, as we'll probably need to change the shell as well.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +850,2 @@
         /* Show OSD */
+        show_osd (manager, "media-eject-symbolic", NULL, -1, -1);

Can you add a:
#define ALL_MONITORS -1
or something like that? Makes the calls easier to read.

::: plugins/power/gpm-common.c
@@ +521,3 @@
+
+        output = get_primary_output (rr_screen);
+        if (output == NULL) {

No need for braces on one-line conditionals.
Comment 5 Cosimo Cecchi 2014-11-13 18:26:31 UTC
Created attachment 290652 [details] [review]
power: add a method to retrieve the backlight output id

We're going to use this later.
Comment 6 Cosimo Cecchi 2014-11-13 18:26:38 UTC
Created attachment 290653 [details] [review]
power: return output id when changing screen brightness

When StepUp() or StepDown() are called to change the screen brightness,
also return the output id where the brightness transition happened (if
any).
This is useful so that the shell can show the OSD only on the relevant
monitor, which will be done in the next commit.
Comment 7 Cosimo Cecchi 2014-11-13 18:26:43 UTC
Created attachment 290654 [details] [review]
media-keys: pass output id to show_osd()

This is currently used by the brightness change OSD.
Comment 8 Bastien Nocera 2014-11-13 18:28:43 UTC
Review of attachment 290652 [details] [review]:

Looks good otherwise.

::: plugins/power/gpm-common.c
@@ +521,3 @@
+
+        output = get_primary_output (rr_screen);
+        if (output == NULL) {

The braces!
Comment 9 Cosimo Cecchi 2014-11-13 18:29:48 UTC
Created attachment 290655 [details] [review]
power: add a method to retrieve the backlight output id

--

Fix the braces!
Comment 10 Bastien Nocera 2014-11-13 18:31:03 UTC
Review of attachment 290655 [details] [review]:

Looks good
Comment 11 Bastien Nocera 2014-11-13 18:32:18 UTC
Review of attachment 290654 [details] [review]:

Looks good.
Comment 12 Bastien Nocera 2014-11-13 18:32:46 UTC
Review of attachment 290653 [details] [review]:

Looks fine.
Comment 13 Cosimo Cecchi 2014-11-13 18:35:05 UTC
Attachment 290653 [details] pushed as 5c17bd0 - power: return output id when changing screen brightness
Attachment 290654 [details] pushed as 8685daa - media-keys: pass output id to show_osd()
Attachment 290655 [details] pushed as a653c34 - power: add a method to retrieve the backlight output id