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

Dark theme in OSX breaks look&feel and text boxes #1751

Closed
4 of 10 tasks
ImOnALampshade opened this issue Feb 20, 2019 · 71 comments
Closed
4 of 10 tasks

Dark theme in OSX breaks look&feel and text boxes #1751

ImOnALampshade opened this issue Feb 20, 2019 · 71 comments
Assignees
Milestone

Comments

@ImOnALampshade
Copy link
Task lists! Give feedback

@ImOnALampshade ImOnALampshade commented Feb 20, 2019

Dark theme in OSX breaks look&feel.

White text boxes also have white text in them, making the app basically unusable.

Details for the issue

What did you do?

Installed & ran app for first time

What did you expect to see?

The screenshot on https://sqlitebrowser.org/

What did you see instead?

screen shot 2019-02-20 at 3 18 00 am

Useful extra information

The info below often helps, please fill it out if you're able to. :)

What operating system are you using?

  • Windows: ( version: ___ )
  • Linux: ( distro: ___ )
  • Mac OS: ( _version: 10.14.3)
  • Other: ___

What is your DB4S version?

  • 3.11.1
  • 3.11.0
  • 3.10.1
  • Other: ___

Did you also

@chrda81
Copy link

@chrda81 chrda81 commented Feb 20, 2019

Running into the same issue 5 min ago. Thanks for posting @ImOnALampshade

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

Yeah, it's a known issue on both MacOS and Windows, that doesn't (yet) seem to have a fix because the GUI toolkit we use (Qt) doesn't yet handle dark mode properly on those platforms. 😦

We have an issue tracking it here: #1493

One potential avenue (for us) would be to recompile the application using Qt 5.12.1 (the very latest release), as things are a bit better there. However that release of Qt has another showstopper bug (#1658) stopping us using it. If/when they fix that, we'd be able to recompile using that, and it'd be a bit better. Still not perfect though... we really need Qt to get "full" dark theme support happening.

The only way I know of getting the application to work on MacOS Mojave... is to turn off Dark mode while using it. And yeah, that's pretty terrible. 😦

Open to suggestions though, just in case we've missed something that would help. 😄

@gerwinbrunner
Copy link

@gerwinbrunner gerwinbrunner commented Feb 20, 2019

Had the same issue

@gerwinbrunner
Copy link

@gerwinbrunner gerwinbrunner commented Feb 20, 2019

Is there a way to switch to the regular theme in the app?

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

I wish. From what I can tell, you need to switch off Dark mode in the mojave System Preferences. eg for all applications. As that's what Qt checks and picks up on. 😦

Someone with deeper Qt or macOS knowledge may know of a workaround, it's just (so far) that's the best we know.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 20, 2019

Wow, this is highly expected and highly broken (except in Linux).

One solution would be to link using Qt 5.12 and work around #1658 by disabling word wrapping.

Another is to use a dark stylesheet and give a preference option:

  • Follow desktop
  • Dark style

I'm testing with https://github.com/ColinDuquesnoy/QDarkStyleSheet
imagen

It isn't perfect, but probably better than what Qt currently does for macOS and Windows.

@justinclift Could we use ColinDuquesnoy/QDarkStyleSheet? Are the licenses compatible?

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

Could we use ColinDuquesnoy/QDarkStyleSheet?

We could probably try it out in an experimental branch + build to see if it improves things.

Looking at that project's README.md, it has C++ code for enabling it:

QFile f(":qdarkstyle/style.qss");
if (!f.exists())
{
    printf("Unable to set stylesheet, file not found\n");
}
else
{
    f.open(QFile::ReadOnly | QFile::Text);
    QTextStream ts(&f);
    qApp->setStyleSheet(ts.readAll());
}

Ideally we'd be able to dynamically switch between using that style sheet (for dark mode), and the standard one (for normal mode). But if dynamic isn't possible, we could just make two macOS builds, one for dark mode and one for normal.

Are the licenses compatible?

That project is MIT licensed, which is compatible with everything. 😄

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

One solution would be to link using Qt 5.12 and work around #1658 by disabling word wrapping.

Yep. I'm ok to create new builds for people to experiment with, if that would help. 😄

mgrojo added a commit that referenced this issue Feb 20, 2019
In order to work around #1658 (QTBUG-73721) the word wrapping is disabled
when compiling with Qt 5.12.x. This workaround will also allow compiling
with better macOS dark theme support by using Qt 5.12 (see issues #1493 and
#1751 and Qt bug reports: QTBUG-68891 and QTBUG-71020) without affecting
builds using previous versions of Qt.
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 20, 2019

Yep. I'm ok to create new builds for people to experiment with, if that would help. 😄

In my last commit I've disabled word wrapping when the Qt version is greater or equal than 5.12.0. This would allow us compiling with Qt without stumbling upon #1658 but without losing word wrapping in unaffected versions. So if we change to Qt 5.12.1, at least for macOS, the macOS dark mode support will be improved. But we have to take into account that https://bugreports.qt.io/browse/QTBUG-71020 is still unresolved, as we've already seen in #1493 (comment).

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

Thanks @mgrojo, I'll make a new build from the master branch using Qt 5.12.1 in our Mojave VM, so people can test it.

Should we experiment with that QDarkStyleSheet thing too?

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

New build is in progress. I don't yet have a good idea of how long builds on that VM take, though I know it's pretty slow. Could take around an hour, but not sure.

@justinclift justinclift changed the title Dark them in OSX breaks look&feel and text boxes Dark theme in OSX breaks look&feel and text boxes Feb 20, 2019
@justinclift
Copy link
Member

@justinclift justinclift commented Feb 20, 2019

@ImOnALampshade @chrda81 @gerwinbrunner @b2397 @matpag @Raistlfiren @spig @jonathanlmarsh Here's a first build using Qt 5.12.1, which should make Dark Mode support a bit better:

With that... what do you reckon?

Note for future use - Seems to take just under 40 mins to create builds on the new Mojave VM. So, can create new builds for future tests without tooooo much delay. 😄

@ImOnALampshade
Copy link
Author

@ImOnALampshade ImOnALampshade commented Feb 21, 2019

This looks much better and fixes the white-on-white text that made it unusable for me when I first installed. So, at least I'm able to do what I needed to.

There's still a couple rough areas - here are screenshots:

Button elements have black text on a dark grey background:
screen shot 2019-02-20 at 9 45 00 pm

The text editor is a white theme, which is fine, but the background of areas with no text is dark grey:
screen shot 2019-02-20 at 9 46 12 pm

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 21, 2019

Thanks @ImOnALampshade. Looks like we're heading in the right direction. Next step, we'll probably investigate that QDarkStyleSheet when @mgrojo has some time to look. Hopefully in the next day(s). 😄

@Nosferican
Copy link

@Nosferican Nosferican commented Feb 21, 2019

What's the solution for downgrading in the meanwhile?

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 21, 2019

Hmmm, just found something weird. The previous release (3.10.1) works ok. It's not in dark mode, but at least all of the elements are readable and usable.

@mgrojo Any idea why 3.10.1 works ok on Mojave - it just looks like a Light mode application - but 3.11.1 doesn't?

If we can just get 3.11.1 to do the same thing (eg work as if it's light mode), that's probably workable for now.

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 21, 2019

Still getting reports (via Twitter) of the breakage on macOS. Seems to be affecting quite a few people. 😦

To hopefully reduce the number of Mojave users hit by this problem, I've added a note to the 3.11.1 blog post, right near the top in a fairly obvious spot:

Note 2 - If you’re using Dark Mode on macOS Mojave, Do Not download
this release. Dark Mode support is broken (badly) on MacOS. It should
(hopefully) be fixed in the next point release.

That should be pretty clear. 😄

@onen5
Copy link

@onen5 onen5 commented Feb 22, 2019

Hit this today. Luckily rolling back to 3.11.0 is safe.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 22, 2019

@justinclift

@mgrojo Any idea why 3.10.1 works ok on Mojave - it just looks like a Light mode application - but 3.11.1 doesn't?

Maybe the older Qt version probably used in 3.10.1 did not even try to follow the macOS theme settings?

Should we experiment with that QDarkStyleSheet thing too?

I'm looking at it, but there are some places where the style sheet is eclipsing our own colour specifications. Don't know why. I can push a branch if experimenting in macOS is welcome.

@ImOnALampshade

The text editor is a white theme, which is fine, but the background of areas with no text is dark grey:

It seems the kind of problem that at least could be solved by Restoring Defaults (under Preferences), if the Qt bug isn't inteferring, at least.

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 22, 2019

I can push a branch if experimenting in macOS is welcome.

Yes, definitely. 😄

I'll probably try a build with the same Qt version as our 3.10.1 release too. If it's as simple as just needing to compile with that, then that's an easy win. 😄

mgrojo added a commit that referenced this issue Feb 22, 2019
A new setting allows to follow the system style or set a new dark style
based on a the style-sheet provided by
https://github.com/ColinDuquesnoy/QDarkStyleSheet
The style-sheet is licensed under the MIT license. Images contained in
that project are licensed under CC-BY license.

Pending issues:

- Use of stylesheets is incompatible to QPalette. Some colours for
previewing settings in the Preferences dialog are eclipsed by the style-
sheet
See ColinDuquesnoy/QDarkStyleSheet#48

- Changing the style should select matching background and foreground
colours for the Browse Data and SQL tabs in Preferences.

See issues #1751 #1493 and #1738
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 22, 2019

I've pushed the dark stylesheet changes to the qdarkstyle branch.

@jsejcksn
Copy link

@jsejcksn jsejcksn commented Feb 23, 2019

Just installed for the first time—this seems incredibly unfortunate, and I'm sorry you guys got bit by this. Here's a fix for anyone who doesn't want to downgrade or run a beta:

  1. Quit the app.

  2. Run the following terminal command:

defaults write net.sourceforge.sqlitebrowser NSRequiresAquaSystemAppearance -bool true

This will force the app to run in the light theme even if Dark is selected in System Preferences > General > Appearance.

To revert the previous command (and allow the app to follow the Appearance mode set in System Preferences):

  1. Quit the app

  2. Run the following terminal command:

defaults delete net.sourceforge.sqlitebrowser NSRequiresAquaSystemAppearance

@justinclift If you want to push an updated release for macOS before things are sorted with Qt, just set this key in the app's Info.plist file. See the section entitled Opt Out of Dark Mode at the bottom of Choosing a Specific Appearance for Your App.

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 23, 2019

@jsejcksn Oh wow, that looks like exactly the thing we need. I'll try that in a Mojave VM shortly, and if it works well (which it seems like it should) I'll rebuild our macOS binaries with it (the updated .plist).

@mgrojo Thanks heaps. Will build a new test binaries using that branch, but probably tomorrow more than today. Feeling burnt out atm, and want to take some time out after getting the above build test done. 😄

@justinclift
Copy link
Member

@justinclift justinclift commented Jun 27, 2020

Awesome, thanks heaps @revolter! 😀

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jun 28, 2020

@revolter
Copy link
Member

@revolter revolter commented Jun 28, 2020

Using the "Follow the desktop style" makes the app show up light, even though my system's style is set to dark.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jun 28, 2020

@revolter
Copy link
Member

@revolter revolter commented Jun 28, 2020

I don't know.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jun 28, 2020

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jun 28, 2020

@justinclift
Copy link
Member

@justinclift justinclift commented Jun 28, 2020

@mgrojo Thanks for pointing that out. I wasn't sure what to check. 😄

That key is definitely in our builds:

<key>NSRequiresAquaSystemAppearance</key>

@mgrojo mgrojo reopened this Jun 28, 2020
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jun 28, 2020

@revolter so, can you test again building without that workaround? If the Qt bug is really fixed, the "Follow dektop style" for the dark system's style should work well now, and we can remove it for the next release.

@revolter
Copy link
Member

@revolter revolter commented Jun 29, 2020

Found an interesting bug (that is only present when the style is set to dark):

image

@revolter
Copy link
Member

@revolter revolter commented Jun 29, 2020

Still doesn't work:

image

diff --git a/src/app.plist b/src/app.plist
index 668cec33..5d76e5c7 100644
--- a/src/app.plist
+++ b/src/app.plist
@@ -76,7 +76,5 @@
 	<string>NSApplication</string>
 	<key>NSHighResolutionCapable</key>
 	<true/>
-	<key>NSRequiresAquaSystemAppearance</key>
-	<true/>
 </dict>
 </plist>
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jun 29, 2020

May that file be installed somewhere from an installation of a release? Or is it included in the program binary? Otherwise, I don't know where the problem might be.

@revolter
Copy link
Member

@revolter revolter commented Jun 29, 2020

Yeah, maybe it's used only when packaging.

@justinclift
Copy link
Member

@justinclift justinclift commented Jun 30, 2020

@revolter From memory, that app.plist is incorporated into the application package (on macOS). The keys in it probably become part of the Contents/Info.plist file in the .app structure.

@revolter
Copy link
Member

@revolter revolter commented Jun 30, 2020

So simply running the app from Qt Creator should use the new contents? Or maybe do I need to do a clean before?

@justinclift
Copy link
Member

@justinclift justinclift commented Jun 30, 2020

Nah, I'm saying that you should be able to download the 3.12.0 release .app, and screw around with the Contents/Info.plist file in there to check. 😄

@revolter
Copy link
Member

@revolter revolter commented Jul 1, 2020

Oooh, I see. Yeah, personally, I think that it looks better than the blue-ish one, but there are still some issues:

Screenshot 2020-07-01 at 10 43 38

Screenshot 2020-07-01 at 10 43 57

Screenshot 2020-07-01 at 10 44 22

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jul 1, 2020

@revolter
Copy link
Member

@revolter revolter commented Jul 1, 2020

It works ❤️ So, we can remove that for the next release, right?

Screenshot 2020-07-01 at 12 48 52

Screenshot 2020-07-01 at 12 49 32

@justinclift
Copy link
Member

@justinclift justinclift commented Jul 1, 2020

Sounds like it. And maybe removed for 3.12.1 too?

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Jul 4, 2020

Sounds like it. And maybe removed for 3.12.1 too?

Yes, I think it should be removed for 3.12.1 too.

@justinclift
Copy link
Member

@justinclift justinclift commented Jul 4, 2020

Just pushed a commit to master removing the setting: 0bb3b23

In theory, that means our nightly builds should be ok now. And we can probably cherry-pick that commit across to 3.12.1 as well.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Nov 27, 2020

This was already confirmed to work right and it is now released in v3.12.1 and in nightly builds, so I think we can close this issue now.

@mgrojo mgrojo closed this Nov 27, 2020
@mgrojo mgrojo added this to the 3.12.1 milestone Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants