search

elementary / wingpanel-indicator-power

Wingpanel Power Indicator
GNU General Public License v3.0
25 stars 15 forks source link

New Horizontal scroll support #204

Closed andirsun closed 3 years ago

andirsun commented 3 years ago

fixes: #201

jeremypw commented 3 years ago

This works most of the time although I did get into a state where the icon (and popover) stopped responding to scrolling - haven't been able to reproduce that issue yet. Also had an issue where scrolling on the indicator in the greeter caused wingpanel to crash and the login to fail.

jeremypw commented 3 years ago

I can confirm that the crash when horizontal scrolling in the greeter is reproducible. I haven't been able to reproduce the login failure thereafter though. The crash does not occur in master.

jeremypw commented 3 years ago

Found another issue: Very fast random scrolling on the icon (all directions at random) can result in the notification popover no longer showing on any scrolling although the brightness still changes.

jeremypw commented 3 years ago

The "handle_scroll_event" code is duplicated from ScreenBrightness.vala - would it be possible to put it in a src/Utils.vala and shared? I think this can be left for another PR though as there is some existing duplication in master.

jeremypw commented 3 years ago

Thats weird I've now discovered that the crash in greeter does occur in master - but with vertical scrolling not horizontal :shrug:. I'll raise an issue.

jeremypw commented 3 years ago

I've now pushed a PR to fix the crash in greeter.

andirsun commented 3 years ago

The "handle_scroll_event" code is duplicated from ScreenBrightness.vala - would it be possible to put it in a src/Utils.vala and shared? I think this can be left for another PR though as there is some existing duplication in master.

Done!

jeremypw commented 3 years ago

@andirsun Great work thank!. I wonder whether infact the Util handler could also do the brightness change? It should have access to the DeviceManager service. There could then be a single correct algorithm to do this and #207 would no longer be required. Sorry for the mission drift - this could be done in a separate PR if desired.

andirsun commented 3 years ago

@jeremypw Done!

jeremypw commented 3 years ago

I'd like to get #210 merged first as it addresses a crash then check this PR still works as expected. If you could test it out on your computer to confirm it works for you I would be grateful. It already has a code approval.

andirsun commented 3 years ago

Sure! Let me test it and report.

jeremypw commented 3 years ago

@andirsun I have fixed the conflicts with master for you and also noticed a flaw in the logic for showing the notification. As it was, the notification did not show until the popover had been created and shown. Now it shows if the popover has never been shown OR it is not currently showing.

jeremypw commented 3 years ago

210 is now merged so this PR can be merged subject to @kvdz satistactory review.

jeremypw commented 3 years ago

@kvdz Are you OK with this PR now?