Pivot
  1. Pivot
  2. PIVOT-245

In Skin json file, add optional coefficients for darkening and brightening base colors

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0
    • Component/s: wtk
    • Labels:
      None

      Description

      Could be useful to add (also in the json file):
      optional coefficients for darkening and brightening base colors, otherwise the default of the theme (in Terra it's + / - 10%) will be used.

      Because on some color schemes the default it's not enough ... I've just seen with my dark scheme.

      1. patch3.txt
        9 kB
        Sandro Martini

        Issue Links

          Activity

          Hide
          Todd Volkert added a comment -

          Sandro, feel free to push this back up to 1.3.1 if you think you can get to it in time. Given that the feedback on Pivot highlights a need for professional-grade documentation, 1.3.1 should ship as soon as the documentation is ship-shape, and any other tickets that got implemented in time will go along for the ride

          Show
          Todd Volkert added a comment - Sandro, feel free to push this back up to 1.3.1 if you think you can get to it in time. Given that the feedback on Pivot highlights a need for professional-grade documentation, 1.3.1 should ship as soon as the documentation is ship-shape, and any other tickets that got implemented in time will go along for the ride
          Hide
          Sandro Martini added a comment -

          Verify if add also a boolean dark (or isDark) attribute (default false and not present in skin json file) but that could help in some cases (in the skin to make some decision on colors).

          Show
          Sandro Martini added a comment - Verify if add also a boolean dark (or isDark) attribute (default false and not present in skin json file) but that could help in some cases (in the skin to make some decision on colors).
          Hide
          Sandro Martini added a comment -

          Greg,
          to finish my work on custom colors I'd prefer to re-target this for the 2.0 (this feature is small and simpler), if no objections (so I could commit it until the end of the week).

          Show
          Sandro Martini added a comment - Greg, to finish my work on custom colors I'd prefer to re-target this for the 2.0 (this feature is small and simpler), if no objections (so I could commit it until the end of the week).
          Hide
          Sandro Martini added a comment -

          For finishing my work on custom colors I'd prefer to resolve also this (helps me with a custom color combination, otherwise it will have low contrast), but before commit I'll post here a patch so other can see.

          Sandro

          Show
          Sandro Martini added a comment - For finishing my work on custom colors I'd prefer to resolve also this (helps me with a custom color combination, otherwise it will have low contrast), but before commit I'll post here a patch so other can see. Sandro
          Hide
          Sandro Martini added a comment - - edited

          This is the patch.

          Note that to NOT change the static methods brighten and darken I have put the new parameter (colorMultiplier) as a static variable inside the class.

          Tell me what do you think on this, and in any case you are free to apply instead of me and change (the variable name "colorMultiplier" probably is not the best).
          If no objections, I'll commit the patch (but not before the end of this week).

          To see that it works and what happens, try to manually changing the default value for example to 0.2 gives a more contrasted theme.

          After this, I'll set the default value even in all other Pivot json themes file (currently there isn't a way to not specify it and catch the exception to leave the default value set in code).

          Verify after this (for 2.1) if it's the case to check the given value in the range 0.0, 1.0 (normalized) and give an exception in case of a given wrong value.

          Show
          Sandro Martini added a comment - - edited This is the patch. Note that to NOT change the static methods brighten and darken I have put the new parameter (colorMultiplier) as a static variable inside the class. Tell me what do you think on this, and in any case you are free to apply instead of me and change (the variable name "colorMultiplier" probably is not the best). If no objections, I'll commit the patch (but not before the end of this week). To see that it works and what happens, try to manually changing the default value for example to 0.2 gives a more contrasted theme. After this, I'll set the default value even in all other Pivot json themes file (currently there isn't a way to not specify it and catch the exception to leave the default value set in code). Verify after this (for 2.1) if it's the case to check the given value in the range 0.0, 1.0 (normalized) and give an exception in case of a given wrong value.
          Hide
          Greg Brown added a comment -

          Why not add a new signature with the multiplier and have the existing signature delegate to it with the default multiplier?

          Show
          Greg Brown added a comment - Why not add a new signature with the multiplier and have the existing signature delegate to it with the default multiplier?
          Hide
          Sandro Martini added a comment - - edited

          > Why not add a new signature with the multiplier and have the existing signature delegate to it with the default multiplier?
          Ok, as suggested you can find the new patch in patch2, and a sample patch for the dark skin, tell me what you think. I add 2 static methods also with the factor parameter and both versions call the adjustBrightness() method. But note that to have this feature retrofit existing behavior, I have keep the static variable (colorMultiplier) ans set its value during json file load.

          If Ok, I can commit this (and after add the default colorMultiplier value in other json custom colors files), otherwise tell me.

          Sandro

          Show
          Sandro Martini added a comment - - edited > Why not add a new signature with the multiplier and have the existing signature delegate to it with the default multiplier? Ok, as suggested you can find the new patch in patch2, and a sample patch for the dark skin, tell me what you think. I add 2 static methods also with the factor parameter and both versions call the adjustBrightness() method. But note that to have this feature retrofit existing behavior, I have keep the static variable (colorMultiplier) ans set its value during json file load. If Ok, I can commit this (and after add the default colorMultiplier value in other json custom colors files), otherwise tell me. Sandro
          Hide
          Greg Brown added a comment -

          You need to add the "colorMultiplier" property to the color scheme files and include them in the patch. Also, can you generate the patch from the root directory instead of the wtk-terra project? Otherwise, looks good.

          Show
          Greg Brown added a comment - You need to add the "colorMultiplier" property to the color scheme files and include them in the patch. Also, can you generate the patch from the root directory instead of the wtk-terra project? Otherwise, looks good.
          Hide
          Sandro Martini added a comment -

          patch3: this should be the final patch (I leave here the patch2 and the other file, but only for historical purposes).
          There are also some little changes to custom colors files (as well as a new file for Swing-like colors).
          Note that the dark colors should be moved here or inside Eclipse it will not be possible to test it with the ColorSchemeCreator ... but it's not a big problem if this is not wanted here.

          After this, probably only few little changes could be done on colors (some combinations are too lighter), to close the related issue PIVOT-579.

          Tell me what you think.

          Sandro

          Show
          Sandro Martini added a comment - patch3: this should be the final patch (I leave here the patch2 and the other file, but only for historical purposes). There are also some little changes to custom colors files (as well as a new file for Swing-like colors). Note that the dark colors should be moved here or inside Eclipse it will not be possible to test it with the ColorSchemeCreator ... but it's not a big problem if this is not wanted here. After this, probably only few little changes could be done on colors (some combinations are too lighter), to close the related issue PIVOT-579 . Tell me what you think. Sandro
          Hide
          Sandro Martini added a comment -

          And another little improvement, maybe for a later release:
          the first color is black, and the second is white, but with the default multiplierCoefficient of 0.1, we have darker and lighter versions of +/- 10%, and in case of these two "extreme" colors, we losetwo of them, because the darker version of black is always #000000, and the lighter version of white is always #ffffff .
          So what do you think to "fix" also these colors in the default json colors ?
          We could have as "base black" #191919, and as "base white" #e6e6e6

          Just to keep track of this.

          Show
          Sandro Martini added a comment - And another little improvement, maybe for a later release: the first color is black, and the second is white, but with the default multiplierCoefficient of 0.1, we have darker and lighter versions of +/- 10%, and in case of these two "extreme" colors, we losetwo of them, because the darker version of black is always #000000, and the lighter version of white is always #ffffff . So what do you think to "fix" also these colors in the default json colors ? We could have as "base black" #191919, and as "base white" #e6e6e6 Just to keep track of this.
          Hide
          Sandro Martini added a comment -

          solved

          Show
          Sandro Martini added a comment - solved
          Hide
          Sandro Martini added a comment -

          Resolved, so now I close it.

          Show
          Sandro Martini added a comment - Resolved, so now I close it.

            People

            • Assignee:
              Sandro Martini
              Reporter:
              Sandro Martini
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development