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

Theming improvements #743

Open
kjk opened this Issue Mar 9, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@kjk
Copy link
Member

kjk commented Mar 9, 2017

@blake-mealey Some preliminary thoughts on theming.

  1. Colors of tabs should be in sync with background/text colors, similar to how vscode works:

vs-dark
vs-white

  1. Changing tab size leaves rendering artifacts that require resizing the window to fix. We should figure out how to fix that or not have variable tab size for now.

  2. Bookmarks menu in dark theme doesn't look good:

bookmarks-dark

Background color is good, text color is too dark.

In light theme, divider window is not visible:

bookmarks-light-divider

  1. I think that's the comment you made on the PR.

Changing colors of the ui (document.textColor, document.backgroundColor) should not be conflated with inverting colors of the PDF file (gRenderCache.textColor, gRenderCache.backgroundColor).

This behavior inherited from current code but to me those use cases are different.

Basically, a PDF has it's own colors. "Invert colors" options is a bit of a hack to re-color PDF bitmaps after it's been rendered. The intent was to provide "dark mode" given the fact that most PDFs are black text on white background.

I think this should be controlled independently from theming. There are two issues:

  1. How to name it? "Invert colors" is not great. "Dark mode for PDF files" is a bit more explicit but still not great. Do other programs have similar functionality? If yes, how do they name it?

  2. What colors should be used? Just go to white text/black background or have it conform to the ui theme?

Also related to that, it seems like we use gRenderCache.backgroundColor for UI where we should use theme colors.

5.) It would be nice to also adopt toolbar to theme colors. There already is support for changing background color in Toolbar.cpp (in handling WM_CTLCOLORSTATIC). We would also have to change text color and edit control colors.

@blake-mealey

This comment has been minimized.

Copy link
Contributor

blake-mealey commented Mar 9, 2017

Good thoughts.

  1. I was basing the "darker" theme off of Visual Studio 2015/2017, which is a bit different:

However, I think it is a good idea to make the initial standard themes very simple and clean. If others want to make more interesting themes later, that's fine. Should I update the dark and light themes to look similar to those screenshots you posted then?

  1. I noticed this, and I agree with keeping consistent tab sizes for now. I think I should make the default tab size slightly bigger for the dark/light themes described in (1) just because it will look better.

  2. I hadn't even seen that menu, I'll look into it.

  3. I just did some research and it looks like Adobe Reader and Foxit Reader both have this feature:


As you can see, they both put it under accessibility settings. Maybe "Contrast Mode" would be a good name for the feature. I suppose contrast mode should be accessible in any theme (even light), but the colours used could still be supplied by the current theme. This would just be to support continuity, since some themes would look weird with, for example, full black backgrounds on the documents. Also, I don't think white text on full black background is very nice to read compared with the colours in VS for example.

I am not quite sure where the gRenderCache comes into this (is it for the document thumbnails?), or how you want it to be handled.

  1. This would be great! It would also be great (although not as necessary) to add theme colours to all of the menus. I have no idea how difficult that would be.

This also brings up the issue that toolbar icons would have to change colours. With the dark theme, toolbar icons would have to be white/light, and with the light theme, they would have to be black/dark. I think this would be best solved by implementing the toolbar that was discussed in issue #734 and using svg icons which we can then decide which colour to draw them with at render time.

I'm very happy to help with all of this, although I don't know how much free time I have coming up (I'm a full time university student and I've just had a light couple of weeks). I also have no clue where to start on (5) which seems like quite a large project, so I would need some guidance/help with it. I'll work on the other points as I have time though :)

@kjk

This comment has been minimized.

Copy link
Member Author

kjk commented Mar 9, 2017

Should I update the dark and light themes to look similar to those screenshots you posted then?

Please do. Also, feel free to add yourself to AUTHORS file.

Make sure to submit PRs relative to the latest master. Given that you've modified your fork's master relative to Sumatra master, the simplest way to get that might be to re-fork.

I am not quite sure where the gRenderCache comes into this (is it for the document thumbnails?), or how you want it to be handled.

gRenderCache is how we implement color swapping in PDF files: they get rendered by render cache and then we change colors in the bitmap based on gRenderCache.backgroundColor etc.

I just noted that in current code we use gRenderCache.backgroundColor in context where we should be probably using theme colors (in Canvas.cpp).

Both Adobe and FoxIt have too much UI for this. For Sumatra maybe a simplified approach that combines menu item e.g. "Replace PDF document colors" that is on/off and settings for providing background replacement/text replacement colors maybe with option to use theme colors.

I'm not convinced there's value in "Use Windows Colors Scheme".

I'm very happy to help with all of this, although I don't know how much free time I have coming up (I'm a full time university student and I've just had a light couple of weeks).

No pressure and I'm happy to help.

@VictorVG

This comment has been minimized.

Copy link

VictorVG commented Mar 11, 2017

In to Git 48c2071 bug #745 is exist, but partial - on some then switched theme to Dark or Darer see original color, or re-rendering have timeout then I see color error #745. I checking x86 and AMD64 build compile use VC++2015 Update 3 (VC++2017 have many problem's - new patch for msbuild $INSTDIR/2017/"vs_edition_name"/$MsBuildDir , bug in VS2017 rand_s() function realization, not have any Registry entries for configuration, e.t.c. ... ).

@VictorVG

This comment has been minimized.

Copy link

VictorVG commented Mar 17, 2017

v3.2 Git fceb4e5 bug #745 is exist, but it's interesting - if you use the settings

FixedPageUI [
SelectionColor = # f5fc0c
WindowMargin = 2 4 2 4
PageSpacing = 4 4
TextColor = # 000000
BackgroundColor = #ffffff
]
EbookUI [
FontName = Georgia
FontSize = 12.5
UseFixedPageUI = true
TextColor = # 5f4b32
BackgroundColor = # fbf0d9
]
ComicBookUI [
WindowMargin = 0 0 0 0
PageSpacing = 4 4
CbxMangaMode = false
]
ChmUI [
UseFixedPageUI = true
]

Then on the Dark / Darker themes the color errors appear chaotically - in one tab there can be shown in others not, if for the EbookUI you put the background xffffff - the reproducibility of the error is 100%, even where it did not seem to exist.

It looks funny, but the bug-reports we sprinkle. :)

@jg-github

This comment has been minimized.

Copy link

jg-github commented Mar 18, 2017

Sorry to be boring, but is there any way to get the nice golden yellow background back, please?

Thanks.

@VictorVG

This comment has been minimized.

Copy link

VictorVG commented Mar 19, 2017

Observations on the assembly v3.2 Git 2de1381 Win32 / Win64

By observing the behavior of the * FixedPageUI = {true | false} option, I noticed that the manifestation of the error "Overlaying the background as a transparent texture on the image" and then distorting its colors for Dark and Darker themes is 100% when this setting is set to true for any From the rendering engines, if the option EbookUI :: UseFixedPageUI = true, switching the themes Light -> Dark or Light -> Darker results in an error on the sdjvuspec.djvu file (it may be incorrectly recognized by the engine) in the form of color distortion, but the reverse switching of the themes This error eliminates the error.

If you set EbookUI :: UseFixedPageUI = false, then the probability of color distortion and subsequent rendering of documents with the output of the colors specified in it tends to zero.
dark
darker
light
light_original

Test kit included executable files Test/7z ...

@JJenkx

This comment has been minimized.

Copy link

JJenkx commented Aug 31, 2017

In Windows, my setup is all black boarders and black start-up screen and total black in full-screen with white text.

Setting/Advanced Options, add line GradientColors to FixedPageUI then choose your hex color. Here is mine setup for all black and white.

MainWindowBackground = #000000
EscToExit = false
ReuseInstance = false
UseSysColors = false
RestoreSession = true

FixedPageUI [
	TextColor = #ffffff
	BackgroundColor = #000000
	SelectionColor = #f5fc0c
	WindowMargin = 2 4 2 4
	PageSpacing = 4 4
	GradientColors = #000000
]

https://i.stack.imgur.com/vMHOQ.png

@blake-mealey

This comment has been minimized.

Copy link
Contributor

blake-mealey commented Sep 26, 2017

@kjk Hello! I have been completely inactive for many months now and I apologize for that. I see that you have (understandably) removed my changes for now since they were very incomplete. I was wondering if they were put somewhere accessible? I am thinking of working on my own fork/branch to get it more stable/correct now that I have some more free time.

@kjk

This comment has been minimized.

Copy link
Member Author

kjk commented Sep 26, 2017

@blake-mealey The code is still there (in Theme.h and Theme.cpp), just not enabled (ENABLE_THEME is not defined).

One of the issues is that the set of colors used is not well defined so I did https://github.com/sumatrapdfreader/sumatrapdf/pull/793/files which centralizes colors in Color.h/Color.cpp.

It's written so that GetAppColor() can use colors from themes.

@blake-mealey

This comment has been minimized.

Copy link
Contributor

blake-mealey commented Sep 26, 2017

Ahh, I see. Now if I was to add themes support to Color.cpp, how would you like that to be done? At what level does it override other options? Should it use the same system as we were before (ex: GetCurrentTheme()->document.canvasColor)?

I think we should move this discussion to somewhere more appropriate, but I would like to really flesh out the requirements for themes in Sumatra. Previously I was just doing my own thing and not really taking in the big picture or thinking about how existing features should be integrated, etc.

@kjk

This comment has been minimized.

Copy link
Member Author

kjk commented Sep 26, 2017

The sketch of how to reach into themes is https://github.com/sumatrapdfreader/sumatrapdf/blob/master/src/Colors.cpp#L254

To me the hard part is to identify and name the colors. I don't think Colors.hpp covers all the colors and I don't think the colors are precisely defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.