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

fix: emit updated on NativeTheme on the UI thread to avoid DCHECK #20137

Merged
merged 3 commits into from Sep 16, 2019

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Sep 5, 2019

Found while experimenting locally, this OnNativeThemeUpdated call can come from any thread apparently so let's post it to the UI thread.,

Related to #19932

Notes: no-notes

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 5, 2019
@codebytere
Copy link
Member

not ok 588 nativeTheme module nativeTheme.themeSource should emit the "updated" event when it is set and the resulting "shouldUseDarkColors" value changes
  AssertionError: expected false to equal true
      at Context.<anonymous> (electron/spec-main/api-native-theme-spec.ts:38:25)
      at processImmediate (internal/timers.js:439:21)

seems this change broke the spec

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 6, 2019
@MarshallOfSound
Copy link
Member Author

seems this change broke the spec

Now the event is async so the previous logic that relied on it being sync doesn't work. Lemme hack around it a bit

@release-clerk
Copy link

release-clerk bot commented Sep 16, 2019

No Release Notes

@trop
Copy link
Contributor

trop bot commented Sep 16, 2019

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

@MarshallOfSound MarshallOfSound deleted the emit-native-theme-on-ui branch September 16, 2019 23:08
MarshallOfSound added a commit that referenced this pull request Oct 8, 2019
…0137)

* fix: emit updated on NativeTheme on the UI thread to avoid DCHECK

* Update atom_api_native_theme.cc

* spec: wait a few ticks for async events to emit so that test events do not leak into each other
jkleinsc pushed a commit that referenced this pull request Oct 8, 2019
* feat: add nativeTheme.themeSource to allow apps to override Chromiums theme choice (#19960)

* feat: add nativeTheme.shouldUseDarkColorsOverride to allow apps to override Chromiums theme choice

* spec: add tests for shouldUseDarkColorsOverride

* chore: add missing forward declarations

* refactor: rename overrideShouldUseDarkColors to themeSource

* chore: only run appLevelAppearance specs on Mojave and up

* chore: update patch with more info and no define

* Update spec-main/api-native-theme-spec.ts

Co-Authored-By: Jeremy Apthorp <jeremya@chromium.org>

* Update api-native-theme-spec.ts

* Update api-native-theme-spec.ts

* Update api-native-theme-spec.ts

* fix: don't expose nativeTheme in the renderer process (#20139)

Exposing these in the renderer didn't make sense as they weren't backed
by the same instance / value store.  This API should be browser only
especially now that we have nativeTheme.themeSource.  Exposing in
//common was a mistake from the beginning.

* fix: emit updated on NativeTheme on the UI thread to avoid DCHECK (#20137)

* fix: emit updated on NativeTheme on the UI thread to avoid DCHECK

* Update atom_api_native_theme.cc

* spec: wait a few ticks for async events to emit so that test events do not leak into each other

* chore: add SetGTKDarkThemeEnabled(enabled) internal helper to allow dynamic theme selection on linux (#19964)

This is just a after-creation setter for the `darkTheme` constructor option.  This is delibrately
a method and not a property as there is no getter.

* spec: remove leftover .only
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.

None yet

3 participants