Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add system theme support #806
base: master
Are you sure you want to change the base?
Add system theme support #806
Changes from 13 commits
49ddefe
6a05e7b
7dfc3ca
c43c420
8e6a4e9
00c8d96
7ebefd6
d2474ae
5dcac4b
ce422a3
0811d4b
5fc4cdd
5ee69bb
1b1092f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identical to base class implemantation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base class raises
NotImpelementedError
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I guess I was looking at the wrong thing
Why not have the default implementation do nothing instead of raising an error? Or just drop the method because there's not an implementation that does anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was generally thinking of the page class as abstract and the functions that
raise NotImpelemented
as 'things derived classes should implement' (even if they are just no-op's). It's just a different paradigm but either way should be equivalent in this context. I don't have strong preferences either way so I can nuke the function if you prefer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of the fork has a PR into the main repo, so once that is merged we might not need to hopefully. TBD. I've tested the PR'd version on all three OS's, though less throughly on Windows as I lack said OS. The version we are using is mostly the same so it ought to work. Feel free to test them yourself but the relevant logic that distinguished between OSes wasn't really the code that changed between the PR'd version and the hardfork version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not the forked version with extra changes?
I don't plan to test this PR. Will you please test it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra changes are copyright, README, and
__version__
, andpyproject.toml
to change the package name. Not really the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are curious, here is the diff stats:
Note the 2 line changes are copyright comments. I don't have access to a Windows device at the moment so I can't really test these changes easily, but they aren't really code changes just the changes necessary to hard-fork a project (changing the name on pypi, copyright, documentation, and version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know how to automate GUI test cases on windows changing system theme and detecting of the colors properly all changed over. Feel free. The actions to test this would be launch app, open preferences, select 'Track System Theme', then toggle the Windows System Theme and visually ensure it tracks the theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to "visually ensure" it, you just need to make sure after the theme is changed the correct behavior occurs in the library. I'm sure applescript and powershell each have ways to adjust the system theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to write these test cases, I can tell you how to do it in applescript (via System Events), I don't know powershell. I imagine for Gnome specifically there is a gsettings option you could find? For detecting if it worked, you'll have to query the QApplication to check that all the colors and such have been updated and also ensure that any visual refresh functions have been called (i.e. redrawing the GUI app).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's on the angr management side, but not necessary for the library itself which is is the subject of this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll happily take a PR for them here: https://github.com/zwimer/darkdetect but I don't have plans to write tests myself for them at the moment. If you do PR them, I'll also merge them into the branch that is PR-ing into the original repo https://github.com/albertosottile/darkdetect so as to avoid actively diverging the hard fork from the original more than necessary; my plan is to get rid of the fork once the PR albertosottile/darkdetect#32 in the original is merged.
Right now that PR has been tested on all three OSes and my fork's master branch doesn't meaningfully diverge from said code. I know the maintainer desired to do more thorough testing before merging the PR though, so I'm sure the test cases would be appreciated by all parties.