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 browser dark/light theme detection #7830

Closed
wants to merge 26 commits into from

Conversation

pmario
Copy link
Member

@pmario pmario commented Nov 1, 2023

@Jermolene -- I think this PR will give users and plugin-authors good flexibility and it is simple.

The configuration is in ControlPanel -> Appearence -> Palette, where it belongs.

  • By default the new configuration can "Enable Light/Dark Mode Handling" to change the setting manually
  • There is a definition for a "Light Palette" and a "Dark Palette"
  • If the configuration should be switched automatically, according to the browser "appearence" setting we need an action-tiddler tagged: $:/tags/DarkLightChangeActions
    • So several plugins can add new actions if needed
  • If the action-tiddler is removed, auto-switching is gone and the menue is for manual switching.
  • All actions tagged: $:/tags/DarkLightChangeActions will be executed
  • There is a docs tiddler + 1 click auto-switching code example
  • updated "change" event listener, because old one is deprecated

These changes are compatible with my PaletteManager preview function and the TiddlyTools PaletteManager (not shown in the screencast below)

light-dark-mode

Copy link

vercel bot commented Nov 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Mar 3, 2024 7:18pm

@pmario
Copy link
Member Author

pmario commented Nov 1, 2023

There is a little problem with all dark themes and the HelloThere page, which should be fixed in a different PR.

@Jermolene
Copy link
Member

Thanks @pmario. This does seem like a reasonable low level primitive for us to provide. I think there might well be other events on the "window" object that we might want to expose in the same way (see MDN docs), notably devicemotion, languagechange, resize, copy, cut, paste, offline, online, hashchange, pageshow and pagehide. For media-match events, we might want to support other queries such as (max-width: 600px).

However, before merging I'd prefer to wait until we have a coherent overall view for the automation of dark mode handling. It may be that the best way forward is a more radical overhaul of our handling of palettes, which I am certainly open to.

Copy link
Member

@Jermolene Jermolene left a comment

Choose a reason for hiding this comment

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

Thanks @pmario I noticed some minor docs style issues

@pmario
Copy link
Member Author

pmario commented Nov 1, 2023

However, before merging I'd prefer to wait until we have a coherent overall view for the automation of dark mode handling.

I think there is are 3rd party plugins at the moment, which implement manual dark/light "toggle button". But that's about it.

The auto-switching is not possible atm, because TW core does not support it. So it's a chicken/egg problem. If we do not implement something like this PR - nothing will happen.

The views about how the configuration UX should look like are diametrically opposed depending on the author, as we could see with our discussion with my first attempt.

It may be that the best way forward is a more radical overhaul of our handling of palettes, which I am certainly open to.

I did ask for community help with the palette colours over at Talk. There may be some other occasions, which I cannot find atm. -- Nobody did do anything.

I do have slightly improved versions of most palettes at the palette-manager edtion. But they are not finished yet.


[. . .] a more radical overhaul of our handling of palettes [. . .]

Does this mean, you'll want to wait merging this PR until this happens?

@Jermolene
Copy link
Member

Thank you @pmario I would like to defer this until we've been able to give more consideration to the user interface.

@linonetwo
Copy link
Contributor

Can auto-switching like VSCode happend in this PR? @pmario seems preview site is not created so I can't test it easily.

I recently find that if you want auto-switching, you shouldn't save $:/palette file when it auto-switching, otherwise this file will change a lot (causing lots of backup, even nothing real is changed)

Also there is a https://github.com/linonetwo/tiddlywiki-switch-theme-user-script as workaround before this is merged, I will integrate it into TidGi APP too, until we have this core feature.

@pmario
Copy link
Member Author

pmario commented Jan 1, 2024

[. . .] @pmario seems preview site is not created so I can't test it easily.

@linonetwo There is a preview version available now.

@pmario
Copy link
Member Author

pmario commented Jan 1, 2024

I recently find that if you want auto-switching, you shouldn't save $:/palette file when it auto-switching, otherwise this file will change a lot (causing lots of backup, even nothing real is changed)

I'm not sure about this. I usually set the default light or dark theme once in a lifetime. So there will be some backups, when the user is experimenting with the new feature. But later on, I do not see why there should be additional switches at all.

IMO it's the same thing as if users set the $:/DefaultTiddlers to [list[$:/StoryList]]. It does not change any content tiddler, but for me $:/StoryList does save an absolutely essential value for the next start of the wiki. So I do not care about backups. It's important that $:/StoryList is saved.

If the number of backups is a problem, IMO the backup strategy has to be changed. Changing the $:/palette tiddler should not cause any problems with backups.

Just my thoughts.

@linonetwo
Copy link
Contributor

@linonetwo
Copy link
Contributor

linonetwo commented Jan 25, 2024

But you are using

\define darkPalette() $:/palettes/GruvboxDark
\define lightPalette() $:/palettes/Vanilla

instead of config tiddler. Can you make it config, and create a control panel for it? I think most of users need this, and most of them don't know "wikiscript".

Why not use action like https://github.com/tiddly-gittly/Modern.TiddlyDev/blob/6a694373f51e6950826f7d839dbfd8403df38567/wiki/tiddlers/%24__Modern.TiddlyDev_Startup.tid#L13

<$action-setfield $tiddler="$:/palette" $value={{{ [{$:/info/darkmode}match[yes]then{$:/configs/palettes/dark}else{$:/configs/palettes/light}] }}}/>

that will use config from control panel.

@kookma
Copy link
Contributor

kookma commented Jul 15, 2024

@pmario
Is it possible to write the color-scheme (e.g. dark/light) into the $:/palette when select dark or light palette?
I ask this because when plugin developer use colored element they need to realize what color scheme/theme is in use so decide to use dark or light colors for plugin element. Right now I have to use a bit longer filter to see what the color scheme is like

[[$:/palette]get[text]get[color-scheme]] :else[[light]]

@pmario
Copy link
Member Author

pmario commented Jul 15, 2024

@kookma [{$:/palette}get[color-scheme]] The get[text]is not necessary.

If the color-scheme field is duplicated, we would have a synchronisation problem.

@kookma
Copy link
Contributor

kookma commented Jul 15, 2024

If the color-scheme field is duplicated, we would have a synchronisation problem.

Actually, it is not duplicated! if the $:/palette stores the name of current palette in use then it can also store the color scheme! But I don't know where else it is used, so as you explained it may cause problem.

The short story is it is not easy for plugin developer to develop plugins with good support of dark/light schemes.

@pmario
Copy link
Member Author

pmario commented Jul 15, 2024

...so as you explained it may cause problem.

As soon as a setting is duplicated, we do have 2 sources of truth, which needs to be managed. As soon as there are 2 sources of truth, they need to be synchronized. So the system needs to make sure, that they are the same. And it needs a mechanism, that deals with cases, where they are not the same. The easiest way to deal with that problem is: Not to duplicate the setting.

The short story is it is not easy for plugin developer to develop plugins with good support of dark/light schemes.

The filter [{$:/palette}get[color-scheme]] is as short as it can be. It should be straight forward to use it. Every colour-palette already has that field.

image

@Jermolene
Copy link
Member

Thanks for your dedication on this @pmario. This is a tough problem to solve given the backwards compatibility constraints, and you've been inventive in trying to accommodate them.

However, I think the results so far are complex and not intuitive. The checkbox that is required to enable light/dark mode handling is a red flag. Firstly, nowhere else in the TiddlyWiki user interface does a checkbox cause additional content to appear or disappear. Secondly, the checkbox is functionally an opt-in to new UI and behaviour that is not backwards compatible. I think this is an anti-pattern.

A lot of the complexity comes from trying to accommodate the existing semantics around $:/palette. I proposed packing both dark and light variants within a single palette as one way to simplify things, but I accept that the packed palettes would have quite a few disadvantages.

A different approach might be to avoid introducing additional config tiddlers by instead defining additional fields to be used on the existing $:/palette tiddler. For example, a dark-palette field could be the title of the optional dark mode palette to use. The presence of that field would enable the automatic switching.

There are some nice advantages to that approach:

  • Old code that just writes to $:/palette will work as expected
  • The user interface can be a simplified version of this PR: by default it shows both a dropdown for choosing the light palette and one for choosing the automatically switched dark palette, with the dark palette unpopulated/blank by default
  • Old code that reads $:/palette won't get the expected results when dark mode is engaged, which means that that @kookma's trick of [{$:/palette}get[color-scheme]] wouldn't work

@pmario
Copy link
Member Author

pmario commented Aug 24, 2024

So

  • We will get 2 new fields $:/palette!!light-palette and $:/palette!!dark-palette for the default configuration

  • $:/palette holds the active configuration -- for compatibility reasons

  • The checkbox and the explanation text can go.

  • @kookma -- plugin authors can easily check, which colour scheme is active: [{$:/palette}get[color-scheme]] -- This filter will still work if the PR is merged

    • There is no need to duplicate the colour-scheme field. It will cause a lot of synchronisation problems if duplicated
    • We should only have 1 source of truth

@Jermolene
Copy link
Member

  • We will get 2 new fields $:/palette!!light-palette and $:/palette!!dark-palette for the default configuration

No. The title of the light palette would be in the text field.

  • $:/palette holds the active configuration -- for compatibility reasons

No, that would lead to the multiple sources of truth that we're trying to avoid.

Just to be sure we're on the same page, with the scheme I proposed the only tiddler whose value would change when there is an automatic switch between dark and light mode is $:/info/darkmode

@pmario
Copy link
Member Author

pmario commented Aug 24, 2024

Just to be sure we're on the same page, with the scheme I proposed the only tiddler whose value would change when there is an automatic switch between dark and light mode is $:/info/darkmode

IMO That's not possible. $:/info/darkmode depends on window.matchMedia("(prefers-color-scheme: dark)") which is defined on TW startup. It sets the $:/info/darkmode tiddler to yes or no, depending on the browser setting.

OS and Browser Dark/light configuration changes trigger a TW change event. This can happen at any time, when the wiki is already running. IMO that's two completely different things.

The $:/info/darkmode can be used with a TW startup action, that modifies the $:/palette tiddler accordingly.

  • We can configure it: If light-mode is defined it does nothing. So "$:/palette" will not be changed.
  • If dark-mode is selected we have to overwrite the $:/palette tiddler otherwise we break all existing code, that depends on $:/palette to hold the currently active palette name:

$:/palette is hardcoded all over the places in the core and within every plugin, that ever extended the functionality of the colour palette handling.

There are several core plugins that depend on the value of the $:/palette tiddler. None of them takes $:/info/darkmode into account.

eg:

None of those elements takes the $:/info/darkmode into account.

@pmario
Copy link
Member Author

pmario commented Aug 24, 2024

There is an other problem, with using $:/palette!!dark-theme as a configuration field. Nobody will respect it. Every existing code will just overwrite the tiddler as a whole. So the config info will be gone.

I do not see a way to go on without additional config tiddlers.

@Jermolene
Copy link
Member

IMO That's not possible. $:/info/darkmode depends on window.matchMedia("(prefers-color-scheme: dark)") which is defined on TW startup. It sets the $:/info/darkmode tiddler to yes or no, depending on the browser setting.

OS and Browser Dark/light configuration changes trigger a TW change event. This can happen at any time, when the wiki is already running. IMO that's two completely different things.

The $:/info/darkmode can be used with a TW startup action, that modifies the $:/palette tiddler accordingly.

I don't understand the relevance of this information. What are the two different things you mention?

I am suggesting that the <<colour>> macro would be modified to use the setting in $:/info/darkmode to help it choose the correct palette. I think you're saying that that wouldn't work, but I don't understand your reasoning.

  • We can configure it: If light-mode is defined it does nothing. So "$:/palette" will not be changed.
  • If dark-mode is selected we have to overwrite the $:/palette tiddler otherwise we break all existing code, that depends on $:/palette to hold the currently active palette name:

I don't understand these two bullet points. It is correct that under my proposal $:/palette would not change when an automatic light/dark mode switch occurs.

I pointed out that my proposal would break code that reads $:/palette, but that is literally the point of my proposal: to see if we can get a more elegant design by dropping some of the backwards compatibility requirements.

$:/palette is hardcoded all over the places in the core and within every plugin, that ever extended the functionality of the colour palette handling.

There are several core plugins that depend on the value of the $:/palette tiddler. None of them takes $:/info/darkmode into account.

Naturally a change like this is going to require visiting all the places that touch this code. I don't see that that is a problem.

@pmario
Copy link
Member Author

pmario commented Aug 24, 2024

I am suggesting that the <<colour>> macro would be modified to use the setting in $:/info/darkmode to help it choose the correct palette. I think you're saying that that wouldn't work, but I don't understand your reasoning.

$:/info/darkmode is set based on the browser setting. But that mode has nothing to do with manual mode. So every function that uses $:/palette has to know if it is OK to use that info.

The enable / disable auto-mode info has to be stored somewhere. That's why the current implementation contains an $:/config/palette/enable-light-dark-detection config tiddler.

If it is set to "no" manual mode is active. It does not matter what $:/info/darkmode says.

I pointed out that my proposal would break code that reads $:/palette, but that is literally the point of my proposal: to see if we can get a more elegant design by dropping some of the backwards compatibility requirements.

In this case I'm absolutely against breaking backwards compatibility. I do not see a possibility that users would not destroy the configuration fields with any of their plugins, that overwrites the $:/palette tiddler.

I personally am not interested in automatic theme switching to dark mode. My mode is light. Always. So I do not see, why I should put even more work into this problem, when I only want a simple switcher between 2 or more light modes.

Naturally a change like this is going to require visiting all the places that touch this code. I don't see that that is a problem.

Time -- and it breaks all existing 3rd party plugins that deal with $:/palette. I did put several 100 hours into the PaletteManager preview system, that makes palette changes easy -- It is stable now -- I would demand a backwards compatible mode for all my palettes.

@Jermolene
Copy link
Member

Hi @pmario what do you mean by "manual mode"?

@pmario
Copy link
Member Author

pmario commented Aug 24, 2024

$:/info/darkmode is set at startup, but once it is starteed I want to switch between light and dark mode manually.

I need:

  • A quick way to switch mode manually, even if the setting is overwritten the next time the wiki is reloaded.
  • A quick way to disable light-dark detection at startup. But I want to keep the startup actions tiddlers in place. I do not want to untag or remove them. So I need a configuration tiddler somewhere.

@Jermolene
Copy link
Member

Hi @pmario it sounds like you don't personally intend to use the automatic switching between dark and light modes, but would be content with two buttons to let you manually switch between two pre-selected palettes. It would not appear to be necessary to make any core changes to offer that feature. It could be seen as a custom version of the palette switcher which just shows palettes that have been selected by the user.

But the original version of this PR was the implementation of an action tag to be executed when switching between dark and light modes. Surely it's the provision of automatic mode switching that has been preoccupying us in this discussion?

Under my proposal, your putative palette switcher would continue to work just at present. Users would encounter two types of incompatibility:

  • If the palette was automatically switched to dark mode, then the dark mode detection fragment shown by @kookma would no longer work. Users could work around the problem by not using automatic dark mode switching
  • Plugins that set the palette would not set the dark mode palette, effectively switching back to a single manual palette. Users coudl work around the problem by using the core facilities to set the current palette

@linonetwo
Copy link
Contributor

linonetwo commented Aug 25, 2024

Is this the current feature requirement?

  1. Config Follow System - startup action and runtime detection
  2. Config Stay in dark / light ("manual mode") - disable both startup action and runtime detection + switch palette immediately
flowchart TD
    AppStart([*]) --> lightModeStay[Light Mode - Stay]
    AppStart([*]) --> darkModeStay[Dark Mode - Stay]
    AppStart([*]) -->|System in Light| lightModeFollow[Light Mode - Follow System]
    AppStart([*]) -->|System in Dark| darkModeFollow[Dark Mode - Follow System]

    lightModeStay -->|System Changes| lightModeStay
    darkModeStay -->|System Changes| darkModeStay

    lightModeFollow -->|System switches to Dark| darkModeFollow
    darkModeFollow -->|System switches to Light| lightModeFollow

    lightModeFollow -->|User toggles Stay Light| lightModeStay
    darkModeFollow -->|User toggles Stay Dark| darkModeStay

    lightModeStay -->|User Follow System / System in Light| lightModeFollow
    darkModeStay -->|User Follow System / System in Dark| darkModeFollow

    lightModeStay -->|Follow System / System in Dark| darkModeFollow
    darkModeStay -->|Follow System / System in Light| lightModeFollow

    style lightModeStay fill:#f9d,stroke:#333,stroke-width:2px
    style darkModeStay fill:#bbf,stroke:#333,stroke-width:2px
    style lightModeFollow fill:#dfd,stroke:#333,stroke-width:2px
    style darkModeFollow fill:#ddf,stroke:#333,stroke-width:2px
Loading
Mermaid is
flowchart TD
    AppStart([*]) --> lightModeStay[Light Mode - Stay]
    AppStart([*]) --> darkModeStay[Dark Mode - Stay]
    AppStart([*]) -->|System in Light| lightModeFollow[Light Mode - Follow System]
    AppStart([*]) -->|System in Dark| darkModeFollow[Dark Mode - Follow System]

    lightModeStay -->|System Changes| lightModeStay
    darkModeStay -->|System Changes| darkModeStay

    lightModeFollow -->|System switches to Dark| darkModeFollow
    darkModeFollow -->|System switches to Light| lightModeFollow

    lightModeFollow -->|User toggles Stay Light| lightModeStay
    darkModeFollow -->|User toggles Stay Dark| darkModeStay

    lightModeStay -->|User Follow System / System in Light| lightModeFollow
    darkModeStay -->|User Follow System / System in Dark| darkModeFollow

    lightModeStay -->|Follow System / System in Dark| darkModeFollow
    darkModeStay -->|Follow System / System in Light| lightModeFollow

    style lightModeStay fill:#f9d,stroke:#333,stroke-width:2px
    style darkModeStay fill:#bbf,stroke:#333,stroke-width:2px
    style lightModeFollow fill:#dfd,stroke:#333,stroke-width:2px
    style darkModeFollow fill:#ddf,stroke:#333,stroke-width:2px

I personally am not interested in automatic theme switching to dark mode. My mode is light. Always.

@pmario I totally understand this, I did so when I bed room did't have another person. But now I have to switch to dark palette automatically when another person's bed time is come, and I continue play my things.

And I see you already implement the "startup action".

@Jermolene We can leave "runtime detection" to next PR, I can do that. Just make sure we can achieve all feature requirements in the end.

@pmario
Copy link
Member Author

pmario commented Aug 25, 2024

And I see you already implement the "startup action".

I implemented everything. All possible modes are covered with the following first 3 steps of the following description

  1. There is an enable configuration that switches auto-mode on or off if the user requires that
  2. There is documentation, which creates a working startup action with 1 click
  3. There is documentation, which creates a working watcher, that listens to browser or OS setting changes, with 1 click
  4. There is a description, what you have to do, if you do not want that browser or OS change triggers dirty mode.

@pmario
Copy link
Member Author

pmario commented Aug 26, 2024

So close this one

@pmario pmario closed this Aug 26, 2024
@Jermolene
Copy link
Member

Hi @pmario I know that it is frustrating when you invest a lot of work in a PR and it doesn't get merged. While there are still a few other issues with this PR there's no doubt that the main blocker has been the difficulties in balancing backwards compatibility with forwards usability. That is always a great challenge, and very hard for the project to avoid if it is to move forwards. My experience is that working through a practical PR is an excellent way to explore the problem, and I think it's worked well here.

On that basis, I am intending to continue working on this, and plan to create an alternate implementation soon. My initial thoughts follow.

As a point of principle, if we're adding a new mechanism we need to generalise it as much as possible. In this case, that means that we shouldn't just provide a hook for monitoring dark/light mode switches: at the very least, we should build a general mechanism for triggering actions when a media query changes. I intend to explore basing the implementation on #7999 which is itself a more general way of triggering actions in response to changes to the wiki and the environment. Together, these new and improved mechanisms would enable other innovations like switching layout when moving from mobile to desktop screen/window sizes.

Thinking about usability, I'm inclined to think that If the user has configured their operating system to switch automatically then it is logical for us to respect that by default (while allowing users to opt out of changes). That would mean that both tiddlywiki.com and the empty edition should by default be configured to respect automatic dark mode switches.

In terms of the necessary configuration and tiddlers, I am thinking that the simplest approach is to represent the state of using automatic platform dark/light mode palette switching by setting $:/palette to a blank value. The only state in the wiki that would change when a platform dark/light mode switch occurs would be the shadow $:/info/darkmode which would not trigger the dirty state.

Rather than a pair of configuration tiddlers that identify the users preferred dark and light mode palettes, I'm inclined to generalise the mechanism to allow any number of palettes to be "favourited" by being given an icon and a short name. Users could set up palettes for "light", "dark" and "dusk" for example. If a platform light/dark mode switch occurs then the system would select the appropriate palette from the favourites list.

I suspect the favourites mechanism might seem unwieldy and baroque, but I think it's the best way to give us an extensible version of the kind of dark/light mode control that users are accustomed to. The favourited palettes would be displayed in a little switcher next to the sidebar chevron, like a miniaturised version of this:

image

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.

5 participants