Community
Participate
Working Groups
This is the top level bug for all the dark theme customization on Windows. I would propose below new system property with much broader scope: org.eclipse.swt.internal.win32.enableCustomDarkTheme=true
Summarizing the key design discussion points taken in below gerrit thread: https://git.eclipse.org/r/#/c/158054/ ------------------------------------------------------------------------------- Here are quick comparison between #setData() and system properties approach: Positives with #setData(): - This way we quietly add experimental feature to SWT. - Full color customization is possible. Negatives with #setData(): - This is not inline with existing SWT design. - This will require code change to client code to adopt the dark theming feature. What this means is in case of any side effects/regressions code change will be needed which we always want to avoid. ------------------------------------------------------------------------------- Positives with system properties: - This will not require any code change in client code to adopt the dark theming feature. - Full color customization is possible with the sub system properties. - In case of any side effects/regressions no code change will be needed and whole feature can be turned off with a single system property.(this is actually a big benefit and that's why we suggested the idea of even pulling the dark scrollbar feature Bug 444560 under this top level flag, anyway it a call we can take later on and under separate thread.) Negatives with system properties: - We expect more of similar customization to follow and the list may grow.(IMO, it's manageable if properly documented in N&N or SWT FAQ page)
I now understand the desire to be able to turn features off without changing code. To reiterate, the last design from Niraj was: // Provides defaults for other properties. org.eclipse.swt.internal.win32.enableCustomTheme=true // Ignored when "enableCustomTheme" is not set org.eclipse.swt.internal.win32.customTheme.Menu.foregroundColor=D0D0D0 org.eclipse.swt.internal.win32.customTheme.Menu.backgroundColor=303030 org.eclipse.swt.internal.win32.customTheme.Menu.Bar.borderColor=505050 In my view, "Ignored when" rule shall be removed. So the design would be: // Provides good defaults for current dark theme org.eclipse.swt.internal.win32.enableDarkThemeTweaks=true // Can be used independently of "enableDarkThemeTweaks" org.eclipse.swt.internal.win32.Menu.foregroundColor=D0D0D0 org.eclipse.swt.internal.win32.Menu.backgroundColor=303030 org.eclipse.swt.internal.win32.Menu.Bar.borderColor=505050 Compared to Niraj's design: 1) It allows to "just get purple menus" without all other customizations (such as dark scrollbars etc). With Niraj's design, gateway property also configures all other properties, causing unwanted effects. 2) It removes unnecessary notion of "customTheme" from properties. With my design, in most cases only "enableDarkThemeTweaks" will be set. This still allows to disable things by removing property. In fact, my design also allows to disable just the feature that is causing (potential) regressions.
Just now, I realized that the desired "disable without changing code" is not going to work anyway. Let's consider a typical application that has at least two themes: dark theme and any other theme. Since the application has two themes, it can't use static VM properties hardcoded in some launcher script. Instead, it needs to activate dark theme from code. So even if it's a system property, it will be set in code. This is already happening: 1) In Eclipse plugin (see Bug 561588). 2) The product I'm working on also sets system property from code. Since the property is set from code, you can't deactivate it without changing code => no benefit from being a system property. The one solution I see here is to have inverse property that disables things even if they are enabled by other means. Please note that this means that all other settings don't have to be system properties and could be .setData() instead. This could get unnecessarily complicated, though.
With all that in mind, my updated suggestion is: For every dark theme tweak, have a pair of: 1) `.setData()` key It will have name that has no notion of dark/custom theme and instead refers to feature directly. Like this name: "org.eclipse.swt.internal.win32.Menu.foregroundColor" This will be used directly from application code. Alternatively, this could be system properties too, but I don't see much point in it, and it makes code somewhat more complicated, because .setData() approach allows to set internal variables and not worry about caching issues. 2) System property with name from (1) with ".disable" appended to it, like "org.eclipse.swt.internal.win32.Menu.foregroundColor.disable=true" When set, this will prevent effect of matching `.setData()`. And do not have any umbrella "set them all", simply due to lack of need. It makes application code much clearer when it explicitly lists configurations wanted instead of relying on SWT "do it all" setting.
By the way, '.setData()' approach also makes code for (2) much simpler, where '.disable' will only need to be tested in '.setData()' itself. This even allows to drop '.disable' string constant entirely, and also allow '.disable' to work for all '.setData()' keys, both current and future, with just two lines of code total. As a reminder, if all things are implemented as system properties, their values can't be cached, because applications (both Eclipse and our product included) typically decide on theme *after* creating 'Display'.
This discussion (spread across many Bugs) already continues longer than 1 year and 3 months, and my patches are still blocked by it. It's easy to imagine that it doesn't make me happy at all. I lost my temper a few times already, and I'm sorry for that. For the constructive continuation of this discussion, I'd like to summarize some code review ideas (mostly stolen from quite enlightening Google guidelines): 1) Objections could be supported by facts or by opinions Facts are always better, because opinions could be very different, depending on specific specialist's background. 2) Objections shall be explained properly Most problems come from objections that are not explained, turning discussion into opinion battle. 3) It's beneficial to explicitly mark nitpicks Nitpicks are low-value requests to improve that can be ignored if author doesn't like to spend time on this. For example, typos in code comments. Marking nitpicks helps avoid a lot of heat. 4) Patch author's opinion shall prevail When two specialists can't agree that usually means that options have roughly equal downsides. In this case, author's opinion shall prevail to maintain healthy motivation for all sides. 5) Make a decision whether the patch makes (in total!) things better or worse If it makes things worse: a) Provide a good explanation and put patch on hold. If it makes things better: a) Try to resolve disputes as long as it doesn't stall. b) Otherwise follow (3) and merge patch. c) Try to provide specific ideas on how to resolve objection. 6) If you object, then exchange replies as quickly as possible Google's research shows that it's not code review objections themselves that upset people, but the delays in the following discussions. I can totally back this. This couldn't take over a year if people didn't object and then forget about the discussion for weeks. So far, the discussion could really benefit from many of these, especially from (6), (5) and (2).
(In reply to Alexandr Miloslavskiy from comment #4) > With all that in mind, my updated suggestion is: > > For every dark theme tweak, have a pair of: > > 1) `.setData()` key > It will have name that has no notion of dark/custom theme and instead > refers to feature directly. Like this name: > "org.eclipse.swt.internal.win32.Menu.foregroundColor" > > This will be used directly from application code. > > Alternatively, this could be system properties too, but I don't see much > point in it, and it makes code somewhat more complicated, because > .setData() > approach allows to set internal variables and not worry about caching > issues. > > 2) System property with name from (1) with ".disable" appended to it, like > "org.eclipse.swt.internal.win32.Menu.foregroundColor.disable=true" > > When set, this will prevent effect of matching `.setData()`. > > And do not have any umbrella "set them all", simply due to lack of need. It > makes application code much clearer when it explicitly lists configurations > wanted instead of relying on SWT "do it all" setting. I suggest that we should not lose track of the original goal to provide a better dark theme on Windows. Ideally we could just go with the best suited color combinations for the dark theme on Windows similar to the dark theme on MAC/GTK and that’s where enabling the umbrella flag to enable the feature comes into picture. Customizing the colors was a specific requirement from your side. 1. With #setData approach we will end up with specifying custom color for every single widget instance and for every widget part in the code, in order to get a uniform themeing experience. Whereas with system property we will have to specify the custom color just once. Right ? 2. Also #setData(foreground/background) approach does look like an alternate to actual widget level APIs like setForeground/setBackground API which again don't fall inline with SWT design. So we will prefer system properties based approach for this work.
(In reply to Alexandr Miloslavskiy from comment #6) > This discussion (spread across many Bugs) already continues longer than 1 > year and 3 months, and my patches are still blocked by it. These patches are new, except for scrollbar patch which was based on an undocumented native support and hence was discussed in great detail on bugzilla. We were very responsive most of the times. > It's easy to imagine that it doesn't make me happy at all. I lost my temper > a few times already, and I'm sorry for that. Alexandr, we appreciate your contributions and I suggest not to take anything personally. See it's not just your time, we also have to put time to evaluate the idea/gerrits. All our comments and feedback are based on technical evaluation of the design.
> I suggest that we should not lose track of the original goal to provide a > better dark theme on Windows. I don't think I'm losing track of it. I'm merely trying to make the API suitable for two different products, because I'm more interesting in the other which is not Eclipse. If I have to chose one of two, that will be ours, apparently. > Ideally we could just go with the best suited color combinations for the > dark theme on Windows similar to the dark theme on MAC/GTK and that’s where > enabling the umbrella flag to enable the feature comes into picture. > Customizing the colors was a specific requirement from your side. Again, because I have two different products in mind. > 1. With #setData approach we will end up with specifying custom color for > every single widget instance and for every widget part in the code, in order > to get a uniform themeing experience. > Whereas with system property we will have to specify the custom color just > once. Right ? Not quite. We could also Display.setData() to avoid configuring every widget. > 2. Also #setData(foreground/background) approach does look like an alternate > to actual widget level APIs like setForeground/setBackground API which again > don't fall inline with SWT design. The same objection could be applied to system properties, so it doesn't help to choose between two approaches. > So we will prefer system properties based approach for this work. So far I heard these reasons for choosing system properties over .setData(): 1) To avoid setting to every control - not correct, because Display.setData() could be used for same effect. 2) "Not in line with SWT design" - honestly I don't understand that because I think we see more .setData() than system properties. 3) To allow to disable settings without changing code - as explained above, this isn't going to work So, if we remove anything which is wrong or applies to both approaches, this is the comparison: --------------------------- #setData(): 1) Good (performance): Allows to cache values in boolean variables instead of querying System.getProperty() and parsing it each time. 2) Good (simplicity): As explained above, just two lines of code are needed to support ".disable" system property for all .setData() keys. 3) Good (clean): Resembles an API more than a collection of key-value strings. System property: 1) WRONG - According to Niraj, allows to disable feature without changing code. 2) UNCLEAR - According to Niraj, better matches SWT design. 3) Good - Allows to turn on (but not off) features without changing code. However, I can't imagine a realistic scenario where that could be of benefit. Eclipse and our product has code-backed support of dark theme. Maybe some other product which by chance uses new SWT but didn't care to enable options for their dark theme? If this is important enough, we could provide an umbrella system property that does necessary Display.setData() in SWT. Common: 1) With proper design, allows fine-tuned configuration of options 2) Many settings are needed to support many options. 3) Not implemented with Widget method, which hints that API is experimental 4) Doesn't need reflection to access in cross-platform code 5) Can be configured from code (which is also the expected usage) My conclusion is that .setData() is better, mostly due to performance and simplicity.
One thing you didn't comment on is the need for umbrella flag that is *required* for other flags to work. As explained above, it doesn't achieve the goal of disabling features in case of regressions. At the same time, it makes things harder if application only wants one feature but not others. In my understanding, umbrella flag shall be removed entirely, or at least shouldn't be a requirement for other flags to work.
> These patches are new, except for scrollbar patch which was based on an > undocumented native support and hence was discussed in great detail on > bugzilla. I had the menubar patch at the same time, but when scrollbars patch was blocked, there was no point submitting another patch because it would clearly be blocked by the same issue. Other patches are newer but still blocked. > We were very responsive most of the times. I guess a lot of different things played part in this unhappy case, and most patches are still blocked. Hopefully it can be resolved soon. > Alexandr, we appreciate your contributions and I suggest not to take > anything personally. See it's not just your time, we also have to put time > to evaluate the idea/gerrits. All our comments and feedback are based on > technical evaluation of the design. I'm not taking at personally. It's just about my work being blocked. If the evaluation takes too long, then I would say that patches should just be merged and specific design can be polished when the discussion comes to its end. Also, can you please explain my status of committer in SWT? Can I commit or no? That feels rather weird.
Hi, I'm rather new in SWT development, but I would like to bring my point of view. (And as I work on a custom menu implementation https://bugs.eclipse.org/bugs/show_bug.cgi?id=562454, i'm quite impacted) First it seems to me there is no design established on how SWT's windows theme should work : - statically (before any SWT elements is created) - almost dynamically (after Display but before widgets) - dynamically (can change any time) From what I see from a eclipse's user standpoint, there is no problem to ask the user to restart its application after changing theme. this is not something you do halfway through a coding session. Then, there is still discussion of how the custom theme should be decided by the client : - not at all (one flag for dark theme, colors decided by swt maintainers) - somewhat (colors for menu, but no for scrollbars?) - totally (lot of properties to have total control over the theme) And how: - setData - System properties - ... again, from my POV and with my little knowledge of SWT : - every color that can be customized should be customizable by clients (maybe few at start and growing from here) - no one client should decide how theme looks from a SWT point of view - setData seems a little bit magic and anti OOP to me but I don't know the constraints - System properties seem fine, but maybe hard for clients to use ? - clients may well want some other way like a Map or a file About (y)our discussion, Don't want to bring a third possibility (but I do) : What about introducing a "Theme" object that would abstract how the colors are set and that widgets could rely upon to know how and when they change ? This way we could have setters, system Properties loaders, classpath file loaders and what not (microprofile-config spec comes to my mind) Already in the solution space, but a singleton & observer pattern seem fit for the solution. Nicolas.
(In reply to Alexandr Miloslavskiy from comment #9) > --------------------------- > #setData(): > > 1) Good (performance): Allows to cache values in boolean variables instead > of querying System.getProperty() and parsing it each time. Same thing can be done for System properties as well: - Making the default color value a static field in Menu.java - And update this color value only if System Property is configured in the static {} block. It would be once per application life-cycle and no need to query/parse the System.getProperty() every time. So, this should address the performance related concerns. > 2) Good (simplicity): As explained above, just two lines of code are needed > to support ".disable" system property for all .setData() keys. > 3) Good (clean): Resembles an API more than a collection of key-value > strings. Yes, but at the cost of getting locked to code change which is not desirable. > System property: > 1) WRONG - According to Niraj, allows to disable feature without changing > code. Yes, SWT allows to disable the feature without changing the code with System property. But at end it depends on client code, how they leverage & benefit from the system property. So, in conclusion it's not a stopping point from SWT side(Eclipse can leverage it any point of time as needed), but with setData() approach it's not possible at all. That's why SWT prefers System Property for all non-API features and I could roughly see 360+ search references to System.getProperty() method call in SWT code base. > 2) UNCLEAR - According to Niraj, better matches SWT design. It's very clear from the JavaDoc of setData()/getData() APIs: Which says #setData() and #getData() are meant for usage of SWT client code only, for which SWT acts as a place holder. Also SWT client code is responsible for it's life-cycle management. Also SWT never consumes the 'raw'/non-API information specified using #setData(). And that's the very reason why #setData() based approach for this feature is not inline with current SWT design. > 3) Good - Allows to turn on (but not off) features without changing code. > However, I can't imagine a realistic scenario where that could be of > benefit. Eclipse and our product has code-backed support of dark theme. > Maybe some other product which by chance uses new SWT but didn't care to > enable options for their dark theme? If this is important enough, we could > provide an umbrella system property that does necessary Display.setData() in > SWT. Yes, allowing to turn-off a feature is very important from SWT point of view, we never know if any future regression(s) is seen that's where the top level System property comes to rescue.
> Same thing can be done for System properties as well: > - Making the default color value a static field in Menu.java > - And update this color value only if System Property is configured in the > static {} block. > It would be once per application life-cycle and no need to query/parse the > System.getProperty() every time. > > So, this should address the performance related concerns. Not quite. I agree that this might work for Menu (I would generally expect that applications configure theme before accessing 'Menu' class for the first time). However, this will certainly not work for 'Display', because it's usually created before configuring theme. This means that system property approach could (at cost of problems if app accesses Menu too early) solve some problems, but not others. > > 2) Good (simplicity): As explained above, just two lines of code are needed > > to support ".disable" system property for all .setData() keys. > > 3) Good (clean): Resembles an API more than a collection of key-value > > strings. > Yes, but at the cost of getting locked to code change which is not desirable. This is wrong, as current system property design will not allow to disable anything. > > > System property: > > 1) WRONG - According to Niraj, allows to disable feature without changing > > code. > Yes, SWT allows to disable the feature without changing the code with System > property. > But at end it depends on client code, how they leverage & benefit from the > system property. Again, this is wrong, because current Eclipse plugin sets system properties, which is done in code, therefore it can't be changed without changing code. > > 2) UNCLEAR - According to Niraj, better matches SWT design. > It's very clear from the JavaDoc of setData()/getData() APIs: > Which says #setData() and #getData() are meant for usage of SWT client code > only, for which SWT acts as a place holder. Also SWT client code is > responsible for it's life-cycle management. > > Also SWT never consumes the 'raw'/non-API information specified using > #setData(). > And that's the very reason why #setData() based approach for this feature is > not inline with current SWT design. Sorry, I didn't understand these. For example, what does it mean that "SWT client code is responsible for it's life-cycle management" ? Let's have a look at GTK implementation which provides per-widget .setData() to attach arbitrary CSS styling consumed by GTK. I would say that this is a very close example to what I'm doing: 1) it allows to configure additional styling 2) styling is specific for one platform only Can you explain why this matches SWT design and my patches do not? > Yes, allowing to turn-off a feature is very important from SWT point of > view, we never know if any future regression(s) is seen that's where the top > level System property comes to rescue. You probably misread what I wrote. This does *not* allow to turn feature off.
(In reply to Nicolas Mahoudeaux from comment #12) > - every color that can be customized should be customizable by clients > (maybe few at start and growing from here) > - no one client should decide how theme looks from a SWT point of view My team agrees here :) > - setData seems a little bit magic and anti OOP to me but I don't know the > constraints To me, .setData() is in fact more oop, because it provides a method, which takes type-checked argument. Strings are just strings, no oop at all. However, I would suggest to not go too deep into this, as it can quickly turn into discussion of wording and stray away from the discussed topic. > - System properties seem fine, but maybe hard for clients to use ? I don't think these are any harder than .setData(), but system properties suffer from downsides I highlighted: 1) Can't be cached reliably, leading to some performance issues. One somewhat incorrect example is that Tree needs to know it for every TreeItem painted! The example is "somewhat incorrect" because it's still possible to cache properties in every individual Tree, but that leads to extra code. 2) More code to support them. > - clients may well want some other way like a Map or a file > > About (y)our discussion, Don't want to bring a third possibility (but I do) : > What about introducing a "Theme" object that would abstract how the colors > are set and that widgets could rely upon to know how and when they change ? > This way we could have setters, system Properties loaders, classpath file > loaders and what not (microprofile-config spec comes to my mind) > Already in the solution space, but a singleton & observer pattern seem fit > for the solution. That would be an overkill. I anticipate some 10 theme-related configuration points only. Also, due to the nature of these theme settings, they are highly platform-specific. This brings to two choices: 1) Theme object will be platform-specific => new problems for cross-platform code which will again have to use reflection to access these. 2) Otherwise, it will have to contain fields for every platform. And on top of that, that will mean that API will probably need to be backwards compatible, but this will cause issues when Win10 dark theme is officially released by Microsoft and SWT workarounds will no longer be needed. How do we delete this official API then? If we do, that will break backwards compatibility. That's why the choice is between two things that are not APIs.
(In reply to Alexandr Miloslavskiy from comment #11) > Also, can you please explain my status of committer in SWT? Can I commit or > no? That feels rather weird. Hi Alexandr, as a committer you can of course commit to SWT. But, that does not give a free pass when other Platform/SWT committers disagree with a change.
(In reply to Alexandr Miloslavskiy from comment #14) > Not quite. I agree that this might work for Menu (I would generally expect > that applications configure theme before accessing 'Menu' class for the > first time). However, this will certainly not work for 'Display', because > it's usually created before configuring theme. This means that system > property approach could (at cost of problems if app accesses Menu too early) > solve some problems, but not others. "solve some problems, but not others." - This is very abstract in the context of the discussion. Static block is class level and IMO is the best time to parse the system property and provide all your custom color configuration. And that's the very usage of Java System property and it's already established concept for such configuration purpose. > This is wrong, as current system property design will not allow to disable > anything. > > Again, this is wrong, because current Eclipse plugin sets system properties, > which is done in code, therefore it can't be changed without changing code. As I already said in comment 13: It was not a stopping point from SWT side at anytime(Eclipse could leverage it any point of time as needed), but with setData() approach it's not possible at all. As per the situation, we can easily get the required System properties behavior in-place to turn 'off' the feature in Eclipse as well via below proposed gerrit change: https://git.eclipse.org/r/#/c/162027 Hope this concern is addressed now. > Sorry, I didn't understand these. For example, what does it mean that "SWT > client code is responsible for it's life-cycle management" ? I tried to summarize what JavaDoc for setData() APIs does, sharing the exact JavaDoc: "Sets the application defined property of the receiver with the specified name to the given value. Applications may associate arbitrary objects with the receiver in this fashion. If the objects stored in the properties need to be notified when the widget is disposed of, it is the application's responsibility to hook the Dispose event on the widget and do so." > Let's have a look at GTK implementation which provides per-widget .setData() > to attach arbitrary CSS styling consumed by GTK. I would say that this is a > very close example to what I'm doing: > 1) it allows to configure additional styling > 2) styling is specific for one platform only > > Can you explain why this matches SWT design and my patches do not? I am not the right person to comment on GTK side of thing but the best I could think of is below possible reason: - System Properties wouldn't have been a feasible way to supply the CSS. --------------------------------------------------------------------------------- Now taking SWT client code into perspective, with setData() approach there are lot of questions opening up w.r.t. when to call the .setData method: - When should .setData() be called before Display or after Display creation, also before Widget creation or after ? - Is dynamic customization of color values required, which can be set at any point in time of widget life-cycle supported? These questions/confusion can be avoided by using a System property, as it’s clear that it has to be set before and is not dynamic.
> "solve some problems, but not others." - This is very abstract in the > context of the discussion. I was trying to say that we have a collection of patches already: 1) Bug 560358 Menu patch - there, system property could be cached in Menu static constructor, but there's some risk that application will use Menu for whatever reason before configuring theme. 2) Bug 560316 Text/etc borders patch - affects 7 classes. In my patch, these are 7 different settings to allow to enable/disable them selectively. Same as (1). 3) Bug 444560 Dark scrollbars - already implemented in Display. The future of (1)(2) is still unclear, but (3) is already implemented in Display. And system properties can't be cached in Display static, because they are configured after Display is created. Therefore, the approach of "cache in static constructor" doesn't solve all caching problems. > Static block is class level and IMO is the best time to parse the system > property and provide all your custom color configuration. And that's the > very usage of Java System property and it's already established concept for > such configuration purpose. This sounds reasonable, but Display case is special, as explained above. Other cases will work, but at risk of caching too early - for example, if theme configuration code in app inspects Menu class, it will cause caching to occur before property is configured. > As I already said in comment 13: > It was not a stopping point from SWT side at anytime(Eclipse could leverage > it any point of time as needed), but with setData() approach it's not > possible at all. No it's possible and will work better. As I already said in comment 4, it will be paired with a system property that disables things. So it will be .setData() to activate and system property to forbid. Why two different approaches? Because activating is expected to happen in every application, but disabling is not expected to happen at all. It's an emergency switch. So enabling can benefit from all the upsides of .setData() such as caching. > As per the situation, we can easily get the required System properties > behavior in-place to turn 'off' the feature in Eclipse as well via below > proposed gerrit change: > https://git.eclipse.org/r/#/c/162027 This argument applies to both approaches, but it applies better to .setData(), because with caching issues in mind, the performance issue becomes doubled in magnitude by testing two properties. > > Sorry, I didn't understand these. For example, what does it mean that "SWT > > client code is responsible for it's life-cycle management" ? > > I tried to summarize what JavaDoc for setData() APIs does, sharing the exact > JavaDoc: > "Sets the application defined property of the receiver with the specified > name to the given value. > > Applications may associate arbitrary objects with the receiver in this > fashion. If the objects stored in the properties need to be notified when > the widget is disposed of, it is the application's responsibility to hook > the Dispose event on the widget and do so." This wording is so generic that I don't understand how it applies to choosing between two approaches. What I see in current usage of .setData() is very similar to what I was trying to do. The reasonable argument will be to show how existing usage is valid, but my is different. > I am not the right person to comment on GTK side of thing but the best I > could think of is below possible reason: > - System Properties wouldn't have been a feasible way to supply the CSS. I don't see why system properties wouldn't work here, except for just one decision: This way, it allows to set styling per-widget and doesn't force you to paint all menus in one color. I feel that you judge about .setData() without looking at how it's actually used in SWT. > Now taking SWT client code into perspective, with setData() approach there > are lot of questions opening up w.r.t. when to call the .setData method: > - When should .setData() be called before Display or after Display creation, > also before Widget creation or after ? This is the same with system properties. You are probably confusing system properties with the idea that they will be set in some configuration file before application is started, but this is wrong both for Eclipse and our product, where theme configuration happens in run time. Since it's the same, it can't be used as argument to decide between approaches. > - Is dynamic customization of color values required, which can be set at any > point in time of widget life-cycle supported? This is the same with system properties. > These questions/confusion can be avoided by using a System property, as it’s > clear that it has to be set before and is not dynamic. As explained above, this is wrong.
> Hi Alexandr, as a committer you can of course commit to SWT. But, that does > not give a free pass when other Platform/SWT committers disagree with a > change. Thanks for clarification. I guess I will wait just a bit more and commit Bug 561444, Bug 562234 etc where there's no discussion at all.
Hi Alexandr, Originally we started this darkTheme feature in SWT: 1. Main use-case for Eclipse: It will always use the best color default@darkTheme(which will be specified in the Widget class). - Activation of dark theme via system property is taken care in your latest patch: https://git.eclipse.org/r/#/c/158054/2/bundles/org.eclipse.swt/Eclipse+SWT/win32/org/eclipse/swt/widgets/Display.java#isCustomDarkTheme() method. 2. Second use-case for your Application(which needs an alternate color combination): Simply configure that alternate color via System Properties: - Pass these custom colors as VM argument - Or same can be configured in your application code, before the creation of Display. 3. Third use-case is option to disable this feature, can be addressed as proposed below: https://git.eclipse.org/r/#/c/162027 With above proposal, we should get a good dark theme feature. ----------------------------------------------------------------------------- But now the 2nd use-case(customization of dark theme colors) from above is getting stretched for dynamic usage: Is dynamic customization of color values required at the cost of UN-necessary complexity and associated risks you mentioned in comment 18 ?
> But now the 2nd use-case(customization of dark theme colors) from above is > getting stretched for dynamic usage: I believe that this is some kind of misunderstanding. Our application does not need to configure *multiple* different settings in runtime. It only chooses one setting on start and stays with it. If user changes theme, restart is required. However, the decision about enabling dark scrollbars etc is taken in run time, based on which theme is configured in application settings. The workflow is roughly like this: 1) Read settings 2) Create display 3) Invoke theme manager to make theme decisions 4) Theme manager enables dark theme tweaks. This is the same for Eclipse. As far as I understand it, Eclipse benefits from dark scrollbars via Bug 561588. Please have a look at https://git.eclipse.org/r/c/160204/ file DarkThemeProcessor.java There, you will see that system property is set in *run time* using 'OS.setTheme()'.
(In reply to Alexandr Miloslavskiy from comment #14) > Let's have a look at GTK implementation which provides per-widget .setData() > to attach arbitrary CSS styling consumed by GTK. I would say that this is a > very close example to what I'm doing: > 1) it allows to configure additional styling > 2) styling is specific for one platform only Since GTK port has already used it for similar purpose, we cannot deny using .setData() on Windows for similar themeing/styling purpose. ---------------------------------------------------------------------- (In reply to Alexandr Miloslavskiy from comment #18) > > As I already said in comment 13: > > It was not a stopping point from SWT side at anytime(Eclipse could leverage > > it any point of time as needed), but with setData() approach it's not > > possible at all. > > No it's possible and will work better. As I already said in comment 4, it > will be paired with a system property that disables things. So it will be > .setData() to activate and system property to forbid. > > Why two different approaches? Because activating is expected to happen in > every application, but disabling is not expected to happen at all. It's an > emergency switch. So enabling can benefit from all the upsides of .setData() > such as caching. After discussion with the team here, let's go ahead with the mix approach for this feature. Thanks!
Just summarizing the feature requirement: 1. Feature will be enabled and disabled via top level system property flag. 2. Once the flag is enabled, we will provide good default colors for dark theme per-configured in the widget classes. 3. Tweaking of the color values happens with the .setData methods approach using String based keys and Color object pairs. Just to clarify, we don't want any more system properties(specifically with the ".disable" appended ones)
With recent discussions in mind, this is the detailed design: 1) The same approach will be used in a collection of patches, such as Bug 560358 Dark Menu Bug 560316 Dark Text/etc borders Bug 444560 Dark scrollbars etc 2) The umbrella property will be: 'org.eclipse.swt.internal.win32.enableCustomTheme' 3) #setData() is the main way of configuring new tweaks. 4) #setData() will be called on 'Display' to avoid configuring every widget. 5) #setData() has no effect with umbrella property set to false. This is the implementation of emergency switch. 6) Already existing 'OS.setTheme()' will now #setData() good defaults Currently it sets '.enableDarkScrollbars' which isn't quite right. This '.enableDarkScrollbars' will be replaced with new design. 7) The new '.enableCustomTheme' umbrella property behaves this way: not set - #setData() can still be used. This is different from previous discussions. false - Emergency switch, see (5) true - Display#Display() will call OS.setTheme(true). The system property is no longer expected to be set in runtime, because #setData() or OS.setTheme(true) shall be used instead. 8) Bug 546859 no longer needed, because OS.setTheme() no longer sets system property and instead calls #setData() which is guarded via (5). I see some possible improvements here: A) It's weird when '.enableCustomTheme=true' enables *dark* theme defaults. If we call it '.enableDarkThemeTweaks', it will be weird when '=false' disables things like purple menus. This is merely a naming issue. However, can we use two properties instead of one? 'org.eclipse.swt.internal.win32.enableCustomTheme' Can only be set to false to disable all tweaks. 'org.eclipse.swt.internal.win32.enableDarkTweaks' Can only be set to true to force OS.setTheme(true) in applications that didn't OS.setTheme(true) from code. B) Do we even need system property to *enable* things when application didn't call OS.setTheme() ? Neither Eclipse nor our product will benefit from it. If we can drop it, then (A) and (7)(8) will be simplified.
(In reply to Alexandr Miloslavskiy from comment #24) > With recent discussions in mind, this is the detailed design: > > 1) The same approach will be used in a collection of patches, such as > Bug 560358 Dark Menu > Bug 560316 Dark Text/etc borders > Bug 444560 Dark scrollbars > etc > 2) The umbrella property will be: > 'org.eclipse.swt.internal.win32.enableCustomTheme' > 3) #setData() is the main way of configuring new tweaks. Current scope for #setData() usage is for themeing purpose only. > 4) #setData() will be called on 'Display' to avoid configuring every widget. > 5) #setData() has no effect with umbrella property set to false. > This is the implementation of emergency switch. > 6) Already existing 'OS.setTheme()' will now #setData() good defaults Good defaults color value needs to be present in individual Widget classes and which will address '.enableCustomTheme=true' use-case, #setData() will give further tweaking options. > Currently it sets '.enableDarkScrollbars' which isn't quite right. > This '.enableDarkScrollbars' will be replaced with new design. Since 'OS.setTheme()' is not an API for client use but it's an internal method for use in Eclipse and can change. So for non-Eclipse SWT users '.enableDarkScrollbars' property is the only public option that's available. - Top level Emergency switch will also apply to scrollbar. Quick check: This scrollbar property is already there, so what's the benefit of making it setData() ? > 7) The new '.enableCustomTheme' umbrella property behaves this way: > not set - #setData() can still be used. > This is different from previous discussions. > false - Emergency switch, see (5) > true - Display#Display() will call OS.setTheme(true). > The system property is no longer expected to be set in runtime, > because #setData() or OS.setTheme(true) shall be used instead. > 8) Bug 546859 no longer needed, because OS.setTheme() no longer sets > system property and instead calls #setData() which is guarded via (5). '.enableDarkScrollbars" will remain. > I see some possible improvements here: > > A) It's weird when '.enableCustomTheme=true' enables *dark* theme defaults. > If we call it '.enableDarkThemeTweaks', it will be weird when '=false' > disables things like purple menus. This is merely a naming issue. > > However, can we use two properties instead of one? > 'org.eclipse.swt.internal.win32.enableCustomTheme' > Can only be set to false to disable all tweaks. > 'org.eclipse.swt.internal.win32.enableDarkTweaks' > Can only be set to true to force OS.setTheme(true) in applications > that didn't OS.setTheme(true) from code. To keep things simple, we should stick to single property name ".enableCustomTheme" to turn on/off. Note: The color will default to dark theme will go into N&N & possibly SWT FAQ. > B) Do we even need system property to *enable* things when application didn't > call OS.setTheme() ? Neither Eclipse nor our product will benefit from it. > If we can drop it, then (A) and (7)(8) will be simplified. Similar reasoning here, since 'OS.setTheme()' is not an API for client use but it's an internal method for use in Eclipse and can change. So Yes, for non-Eclipse SWT users we will need system property to *enable* things.
> Current scope for #setData() usage is for themeing purpose only. OK > Since 'OS.setTheme()' is not an API for client use but it's an internal > method for use in Eclipse and can change. So for non-Eclipse SWT users > '.enableDarkScrollbars' property is the only public option that's available. > - Top level Emergency switch will also apply to scrollbar. > > Quick check: > This scrollbar property is already there, so what's the benefit of making it > setData() ? So that a single approach is used for theming tweaks instead of having a collection of different approaches. The will also be a minor caching benefit as a result. That is, not having to check 2 system properties each time SWT needs to know if dark explorer theme is active. > Similar reasoning here, since 'OS.setTheme()' is not an API for client use > but it's an internal method for use in Eclipse and can change. > > So Yes, for non-Eclipse SWT users we will need system property to *enable* > things. Did we reach agreement that it shall not be required to set top-level property to true in order to use .setData() ? Requiring it to be set doesn't seem to give any benefit, but makes things more complicated. For instance our product will have to make sure to override everything to our own values, and we'll have to keep an eye on not receiving unexpected unwanted tweaks when SWT changes. As a reminder, the emergency switch is when top level property is set to false. If yes, then what's the benefit of having a system property that provides defaults? Eclipse will not benefit from it, because it uses OS.setTheme(). Our product will not benefit from it, because we want different values and will use .setData() anyway. Any other product will probably also want to use different defaults. So it seems that noone wants top-level property that gives defaults. The only reason I see is some theoretical application that uses new SWT but didn't activate dark theme, and users could theoretically use top level property to activate it manually. Is that scenario worthy of adding additional complexity? If we can agree that there's no need to set property to true, then the property can be narrowed to only serve as emergency switch off and receive a better name (along with simplifying code).
(In reply to Alexandr Miloslavskiy from comment #26) > > Quick check: > > This scrollbar property is already there, so what's the benefit of making it > > setData() ? > > So that a single approach is used for theming tweaks instead of having a > collection of different approaches. The will also be a minor caching benefit > as a result. That is, not having to check 2 system properties each time SWT > needs to know if dark explorer theme is active. Ok fine go ahead, but then we will have to remove the N&N entry that went against the scrollbar bug 444560 in 4.16 M1 > > Similar reasoning here, since 'OS.setTheme()' is not an API for client use > > but it's an internal method for use in Eclipse and can change. > > > > So Yes, for non-Eclipse SWT users we will need system property to *enable* > > things. > > Did we reach agreement that it shall not be required to set top-level > property to true in order to use .setData() ? Requiring it to be set doesn't > seem to give any benefit, but makes things more complicated. For instance > our product will have to make sure to override everything to our own values, > and we'll have to keep an eye on not receiving unexpected unwanted tweaks > when SWT changes. Yes and for false comment 24, point 5 applies. > As a reminder, the emergency switch is when top level property is set to > false. Right > If yes, then what's the benefit of having a system property that provides > defaults? Eclipse will not benefit from it, because it uses OS.setTheme(). > Our product will not benefit from it, because we want different values and > will use .setData() anyway. Any other product will probably also want to use > different defaults. So it seems that noone wants top-level property that > gives defaults. The only reason I see is some theoretical application that > uses new SWT but didn't activate dark theme, and users could theoretically > use top level property to activate it manually. Is that scenario worthy of > adding additional complexity? If it's making things complex, we are fine dropping the "true" use-case of the system property, only if you achieve below: For Eclipse purpose we will need good defaults from SWT side, which must now come via OS.setTheme(true) method implementation. > If we can agree that there's no need to set property to true, then the > property can be narrowed to only serve as emergency switch off and receive a > better name (along with simplifying code). Sure go ahead, just a reminder we are into last week of development for M3, next week will be M3 testing.
OK good, I will rework patches soon.
New Gerrit change created: https://git.eclipse.org/r/162888
Gerrit change https://git.eclipse.org/r/162888 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a9dca4fe20f3cece4ee5a10483591d52bf4f727d
New Gerrit change created: https://git.eclipse.org/r/163196
Gerrit change https://git.eclipse.org/r/163196 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=c9cfc11cb0c2c3c9f1cf1880fe8d587600943046
New Gerrit change created: https://git.eclipse.org/r/163358
Gerrit change https://git.eclipse.org/r/163358 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=4b6fda41ded44fe272879a0b1c1f15b253942325
Resolving this top level plan item bug for 4.16. If there are any issues noticed, please open new bugs.
