Restore GNOME cursor theme on sugar DE exit

srevinsaju commented 4 years ago

Fixes #872 & Fixes #827 @walterbender @quozl Please review

quozl commented 4 years ago

Thanks.

[x] store the old value in memory, not in a file; there's no need to store it in a file unless you are also handling the situation where Sugar does crash, and if you're doing that you'll need to ignore the value 'sugar' if you find it currently set,
[x] don't instantiate the same Gio.Settings so many (3, yes three) times, cache it instead.

srevinsaju commented 4 years ago

@quozl Fixed it,. Please review

quozl commented 4 years ago

Thanks. Reviewed.

[x] new global variable settings is specific to one settings, yet others are used in the source, see all references to Gio.Settings, this can be confusing,
[x] why a dictionary for the cursor? isn't a single object enough?
[x] what is e in the warning, and why is a warning shown?
[x] the number of lines added seems excessive.

srevinsaju commented 4 years ago

@quozl

[x] new global variable settings is specific to one settings, yet others are used in the source,

see all references to Gio.Settings, this can be confusing, Ok, I had to declare Gio.Settings for mouse cursor on stop_window_manager.

[x] why a dictionary for the cursor? isn't a single object enough?

I expected to extend the cursors to other DE's which uses GTK or other forks of GNOME like CInnamon, Budgie which doesn't inherit from Gio.settings-customtheme but uses some other methodology. I don't use CInnamon or Budgie, so well, can't test it.

[x] what is e in the warning, and why is a warning shown?

Removed it. Used to instantiate user with message, custom cursor theme could not be received from Gio

[x] the number of lines added seems excessive.

Thats because I created two new functions to handle the events. As mentioned above, I thought it would be useful to extend the existing functions.

srevinsaju commented 4 years ago

@quozl made changes on the basis of IRC messages: https://github.com/sugarlabs/sugar/pull/897/commits/a358d333f434348d0d6a0235bf16c92b079af2f2

quozl commented 4 years ago

Thanks. New flake8 warnings added;

src/jarabe/main.py:199:1: E302 expected 2 blank lines, found 1
src/jarabe/main.py:202:1: E112 expected an indented block
src/jarabe/main.py:202:3: E999 IndentationError: expected an indented block
src/jarabe/main.py:206:1: W293 blank line contains whitespace

[x] prefix the global variable with _ like the others, sorry didn't notice that before,
[x] put the global declaration after the function declaration,
[x] don't really need those comments,
[x] still not sure what the point is of testing the variable; it should never be None, (if the GNOME schema is missing, Sugar won't start),

srevinsaju commented 4 years ago

[x] prefix the global variable with _ like the others, sorry didn't notice that before,

[x] put the global declaration after the function declaration,

[x] don't really need those comments,

[ ] still not sure what the point is of testing the variable; it should never be None, (if the GNOME schema is missing, Sugar won't start),

Yes, in the unlikely case of getting an empty string, it would be incorrect to assign the custom_cursor_theme as 'None', if it ever happens. It would be better to terminate sugar with sugar-cursor unchanged.

quozl commented 4 years ago

Yes, in the unlikely case of getting an empty string, it would be incorrect to assign the custom_cursor_theme as 'None', if it ever happens. It would be better to terminate sugar with sugar-cursor unchanged.

An empty string is fine. If we get an empty string on entry, we should set an empty string on exit, not default.

srevinsaju commented 4 years ago

@quozl, thanks for that info. fixed it on https://github.com/sugarlabs/sugar/pull/897/commits/6be7c69e8754be0b878411e56cfeddfe1a29e2a0

quozl commented 4 years ago

Thanks. Reviewed. Commits added to keep style and minimise change.

quozl commented 4 years ago

Probably the last remaining thing;

[x] per the issue, and my first review, _keep the Gio.Settings instance instead of deleting at end of the start_windowmanager function, this is so that time is not wasted setting up and tearing down the Gio.Settings instance; faster logout or shutdown, important if the shutdown is being driven by depleted battery.

srevinsaju commented 4 years ago

@quozl have fixed the final correction on https://github.com/sugarlabs/sugar/pull/897/commits/0b89bb2577c84001b5a029447ea6650b91440cd7 .

quozl commented 4 years ago

Thanks. Reviewed. Tested on Ubuntu 19.04 with Sugar 0.116.

srevinsaju commented 4 years ago

Thanks @quozl for merging it. I was away for a while :)

sugarlabs / sugar

Restore GNOME cursor theme on sugar DE exit #897