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

Add a custom theme dialog #662

Merged
merged 13 commits into from
Jul 6, 2022
Merged

Add a custom theme dialog #662

merged 13 commits into from
Jul 6, 2022

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Jul 1, 2022

Pulled from #552
Fixes #418

Potentially address #39
Potentially addresses #71
Potentially address #501
Partially addresses #507
Potentially address #632

Additional work:

  • Fix appearance of dialog under RTL
  • Add a reset to default palette button
  • Name variables according to linked nominal color
  • Add labels for light colors
  • Tweak layout and add some margins
  • Persist user custom theme after switching to a builtin theme
  • Import a theme

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 1, 2022

Screenshot from 2022-07-01 15 55 48

Screenshot from 2022-07-01 16 02 16

@jeremypw jeremypw marked this pull request as ready for review July 1, 2022 15:03
@jeremypw jeremypw requested a review from danirabbit July 1, 2022 15:04
@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 1, 2022

@danrabbit I would appreciate your comments on this. I added the extra labels as it didn't seem obvious to a new user why there were two very similar color buttons with only one label.

The main flaw with this is that it is not possible to save a custom theme permanently - if you switch back to a builtin theme it overwrites your custom theme and you cannot return to it without rebuilding it. It might be better to fix that in a new PR as there is more than one possible solution - e.g. a new setting or a file in the user home.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 1, 2022

I see from the prior art in #418 that no-one else thinks the extra labels are necessary so I'll remove if required.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 1, 2022

There are also some possible extra refinements like using palette colors for foreground/background but that can be done in another PR.

@danirabbit
Copy link
Member

I think we should solve the issue of persisting the custom palette before exposing this toggle so that we don't have an immediately broken feature. We should probably make sure that the palette setting is only used to store values for the custom theme and that Terminal is loading the palette for built-in themes from the Theme class and not from gsettings

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 2, 2022

Yeh, that makes sense.

@jeremypw jeremypw marked this pull request as draft July 2, 2022 13:00
# Conflicts fixed:
#	src/Widgets/TerminalWidget.vala
@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 3, 2022

@danrabbit After merging master and making relevant changes, the custom theme now persists. I have moved the "Color Palette" header to before the background/foreground buttons because although the palette strictly speaking does not include these, to the average user they are just some more color settings. I have also changed the reset button to reset the foreground/background/cursor colors for the same reason.

Can importing/saving custom themes be left for a later PR, if required?

@jeremypw jeremypw marked this pull request as ready for review July 3, 2022 09:40
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Two small questions, but other than that totally happy with this as an initial implementation. Let's get this in and then smooth out the flow a little bit/do other features in future branches

src/Themes.vala Outdated Show resolved Hide resolved
src/Themes.vala Outdated Show resolved Hide resolved
src/Dialogs/ColorPreferencesDialog.vala Outdated Show resolved Hide resolved
jeremypw and others added 2 commits July 5, 2022 19:29
Co-authored-by: Danielle Foré <daniel@elementary.io>
@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 5, 2022

I have implemented your suggestion re fallback palette as it simplifies the code and the issue of possible invalid colors in the settings (presumably due to manual editing) is unlikely. We now fallback to a default palette if any of the custom colors are invalid rather than replacing only the invalid color.

@jeremypw jeremypw requested a review from danirabbit July 5, 2022 18:57
src/Themes.vala Outdated Show resolved Hide resolved
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

One suggestion and a comment

src/Themes.vala Show resolved Hide resolved
@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 6, 2022

Ok, so you want to keep correcting individual colors; I'll revert to that. I just thought that this pretty unlikely to be required in practice so simplified it. I guess if we implementing importing customs themes it raises the risk of invalid colors being imported.

Co-authored-by: Danielle Foré <daniel@elementary.io>
@jeremypw jeremypw requested a review from danirabbit July 6, 2022 17:36
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Yeah it's just a little bit less destructive. This looks good!

@danirabbit danirabbit merged commit 137bbc1 into master Jul 6, 2022
@danirabbit danirabbit deleted the custom-theme branch July 6, 2022 17:39
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.

User-provided color scheme
2 participants