Skip to content
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

VPN-6213 (part 6): Restructure files, add linter checks #10054

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

mcleinman
Copy link
Collaborator

Description

This is the final PR for restructuring our colors to allow for additional theming. This builds on #10028, which built on #10019, which in turn built on #10015, which in turn built on #10014, which in turn built on #9983. All 5 have been merged into main.

In this PR (roughly in order of commits):

  • Files are reworked
    • theme.js is renamed to sizing.js
    • the colors.js file is broken out into 3 files: colors.js (identical for all themes), theme.js (customized for each theme), and derived-theme.js (identical for all themes)
    • The 2 color files identical for all themes were moved up to the main theme directory
    • There is a hacky thing at the top of colors.js and the bottom of derived-theme.js to allow us to merge all 3 color files into one before trying to read it as a JS object. This is well-commented. (Without this, I was having trouble letting theme.js refer to colors named in colors.js.)
  • theme.cpp is reworked to match the restructured files
  • Added a sample of another theme. (It's very ugly, and I didn't touch all the colors - just the very basic colors to make a sample, very imperfect dark mode.)
  • Added a python file to make sure our colors don't stray from the intention. It will run on every PR via the PR linting work. I've tested all of it's checks by manually changing color and QML files. This file:
    • Ensures that theme.js files and derived-theme.js do not contain any hex color codes.
    • Ensures that all theme.js files use the exact same list of theme-defined colors.
    • Ensures there are no explicit add hex color codes in any QML file.
    • Ensures that all MZTheme.colors.x colors in all QML files come from either theme.js or theme-derived.js (to make sure we don't use anything like MZTheme.colors.red in a QML file).

To see the secondary theme, use the developer debug menu's theme selection menu.

Reference

VPN-6213

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@mcleinman mcleinman force-pushed the vpn-6213-part-6-restructure-files branch from 90684ec to 18eeab0 Compare November 19, 2024 21:02
@mcleinman mcleinman force-pushed the vpn-6213-part-6-restructure-files branch 2 times, most recently from de38a21 to e0f7735 Compare November 19, 2024 22:15
@mcleinman mcleinman force-pushed the vpn-6213-part-6-restructure-files branch from e0f7735 to eb8ce7a Compare November 19, 2024 22:16
scripts/ci/check_colors.py Show resolved Hide resolved
src/theme.cpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really love the cleanup that you did with this.

WDYT: would it be possible to codify a theme now in c++? Like QObject -> BaseColor -> MainColor and have the Theme.cpp simply expose the current color QObject as property?
Because with your nice cleanup i dont see anything the jsengine does the c++ preprocessor and a few consexpr could not do. And it's probably faster to expose a static QObject data vs concatting a string, evaling it in js and then exposing the static data.
Note not for now, this looks great but as followup task in our backlog. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I got into this, I found myself unsure why we were using a JS object to start with. (Because it's only QML actually using it?) I agree it's slower, though luckily it's only loaded at the boot of the app.

If it's possible to keep this structure (base colors that cannot be used in the app, theme colors that are used in app but only refer to base colors, derived theme colors that can be used in the app and refer to theme and base colors), I'm happy to move this into C++.

scripts/ci/check_colors.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@oskirby oskirby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! This is awesome!

@lesleyjanenorton lesleyjanenorton merged commit 2e526b6 into main Nov 20, 2024
116 of 117 checks passed
@lesleyjanenorton lesleyjanenorton deleted the vpn-6213-part-6-restructure-files branch November 20, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants