Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

[UI] Charts Enhancements: Themes, DPI, legend hiding #4288

Closed
hreichert opened this issue Sep 17, 2017 · 32 comments
Closed

[UI] Charts Enhancements: Themes, DPI, legend hiding #4288

hreichert opened this issue Sep 17, 2017 · 32 comments

Comments

@hreichert
Copy link
Contributor

hreichert commented Sep 17, 2017

I would like to add some enhancements to the ChartServlet, DefaultChartProvider, REST API, BasicUI/ClassicUI to support the following things:

  • Chart themes for different looks
  • dpi parameter for dynamic sizes based on DPI
  • Automatic showing/hiding of the legend based on number of chart series
  • legend parameter to hide/show the chart legend

Theme support

Chart Servlet

My change extends the ChartServlet with a new GET parameter theme, like this:
/chart?groups=gChart&theme=dark
The theme parameter is optional.

New interface ChartTheme for the DefaultChartProvider

A new interface that themes for the DefaultChartProvider can implement.
As the javadoc for ChartProvider suggests, this is only used for this concrete chart provider and so the interface is in the org.eclipse.smarthome.ui.internal.chart.defaultchartprovider package.

New themes:

  • ChartThemeBright (default theme) - implements the current colors and font sizes
  • ChartThemeDark - darker version
  • ChartThemeBlack - mostly black / very dark version

The themes can also calculate values (font sizes, line widths) based on the DPI value. See below.

Compatibility

This behaviour introduces no changes to existing users of charts:

  • The theme parameter is optional at all places
  • If no theme parameter is given, the default theme "bright" is rendered
  • The "bright" theme has exactly the same colors as the current look

DPI support

Regarding to #4284 it would be a great improvement to have dynamic font sizes.
Because different font sizes are used in the same chart, a single "fontsize=10px" parameter would not be sufficient, especially if we take different screen resolutions / DPI into account.

A new dpi parameter is now used to calculate font sizes, the width of the chart series lines and the grid lines.

ChartServlet

My change extends the ChartServlet with a new GET parameter dpi, like this:
/chart?groups=gChart&dpi=320
The dpi parameter is optional. Default is 96.

Compatibility

This behaviour introduces no changes to existing users of charts:

  • The dpi parameter is optional at all places
  • If no dpi parameter is given, a default of 96 is used
  • The calculations are based on 96 dpi to deliver same sizes as currently

Legend hiding

Two changes:

  • Legend is automatically hidden, if there is only one chart series (-> one item) - otherwise the legend is shown
  • legend=true/false parameter for the ChartServlet and the Sitemap model to force hiding or showing the legend

ChartServlet

New GET parameter legend.
/chart?items=Temperature_Livingroom&legend=true
The legend parameter is optional. No default -> automagic, see above

Sitemap model

New optional attribute legend for the chart sitemap model.
One can define a chart like this:
Chart item=gChart period=h legend=false
Chart item=Temperature_Livingroom period=h legend=true

BasicUI / ClassicUI

The BasicUI / ClassicUI takes the optional legend attribute from the sitemap model into account and constructs a matching url to the ChartServlet. If the parameter is not given, it's not sent to the Servlet, so the above mentioned automagic is used.

REST API

The REST API delivers the legend attribute in the chart sitemap model, if it is set.

Compatibility

This behaviour introduces a change to existing users of charts:

  • The legend parameter is optional at all places - so no issue here
  • If the user has a chart with only one single item, the legend is now hidden by default
  • If the user has a chart with more items, there is no change in behaviour

Renderings

Default theme "bright", default settings:
chart_1
Theme "dark", default settings:
chart_2
Theme "black", default settings:
chart_3

Thinking further

These features can be used to improve the experience with the OpenHAB android app:

  • app sends dpi parameter -> better readable charts on hi-res smartphones
  • user can tap on a chart to hide the legend

Maybe also the new BasicUI theme mechanism (#4137) can use the theme parameter to select a matching chart theme for the WebUI theme.

@mueller-ma
Copy link

Do we really need the sitemap configuration? I might be better when every client choose the theme itself.

@hreichert
Copy link
Contributor Author

hreichert commented Sep 17, 2017

Good point. I thought of that too and came to this conclusion:

If the user absolutely wants a specific theme, he/she can define it in the sitemap with theme=XXX.
If the user does not care, he/she would not define a theme.

The "client", like the OpenHAB android app, would check if there is a theme defined for this chart.
If yes: take THIS theme - the user wants it
If not: take a theme that matches the current layout of the client. e.g. take "black" theme for the "Black/AMOLED theme" in the OpenHAB android app.

I absolutely had the andoid app in mind while working on this change, because every night when I turn on my smartphone to check my room temperatures in the app, my retina gets burned with the bright white charts ;)

Other opinions?

Edit: Removed theme parameter from the sitemap model and REST API

@mueller-ma
Copy link

mueller-ma commented Sep 17, 2017

If the user absolutely wants a specific theme, he/she can define it in the sitemap with theme=XXX.

Than he should change the theme of his client or open an issue if that is not available.

If not: take a theme that matches the current layout of the client. e.g. take "black" theme for the "Dunkel Thema" in the OpenHAB android app.

There should be enough themes, e.g. blue bright, blue dark, red bright, red dark, ...

@hreichert
Copy link
Contributor Author

Ok, I will rethink this. Maybe it's better to work only on the ChartServlet/DefaultChartProvider within this issue and keep the other sitemap stuff seperate, if at all.

What do you think about a "fontscale" parameter for the ChartServlet?
Current font sizes:

  • Axis Labels: 11
  • Legend: 10

Lets assume the "fontscale" parameter gives the scaling in percent.

  • fontscale = 130
    • Axis Labels: 14
    • Legend: 13
  • fontscale = 200
    • Axis Labels: 22
    • Legend: 20
  • fontscale = 50
    • Axis Labels: 6
    • Legend: 5

@mueller-ma
Copy link

Sounds good

@hreichert
Copy link
Contributor Author

Regarding you suggestion for a new parameter "hide-legend" from openhab/openhab-android#373
How shall we name it?
I can think of:

  • hidelegend - default false
  • hide-legend - default false
  • showlegend - default true
  • show-legend - default true

But this parameter is a candidate for the sitemap chart element, isn't it?

@mueller-ma
Copy link

It might be good for a workaround for openhab/openhab-core#80, e.g. click on the chart to toggle legend.

@hreichert
Copy link
Contributor Author

Other use case: each of my temperature sensors (bedroom, living room, ...) has it's own chart. They are in own frames with a headline and the concrete temperature item. So I know what the chart shows and I don't want a seperate legend that hides information.

@hreichert
Copy link
Contributor Author

hreichert commented Sep 17, 2017

Ok so this is an example with the following parameters:

/chart?items=ChartNumber1&period=h&w=500&h=250&theme=black&fontscale=100&hidelegend=true

chart1

/chart?groups=gChart&period=h&w=500&h=250&theme=black&fontscale=150&hidelegend=false

chart2

And the default:

/chart?groups=gChart&period=h&w=500&h=250"

chart3

@mueller-ma
Copy link

You should adjust the graph colors in the dark theme. Blue is not very good readable

@mueller-ma
Copy link

Another option: remove-grid

@hreichert
Copy link
Contributor Author

Yep, colors are not finished yet. I should ask some persons that have more taste than me to review the colors ;)

@mueller-ma
Copy link

Consider taking the "500" colors: https://material.io/guidelines/style/color.html

@mueller-ma
Copy link

Another idea: transparent background as seperate option.

@hreichert
Copy link
Contributor Author

Thanks for your suggestion regarding colors, will try these.
My try is to keep the "base colors" mostly the same between the "bright" and the "dark"/"black" theme, so that every chart line has matching colors at least in these themes.
I don't want that in my temperature chart the bedroom is orange in one theme, and cyan in the other.

@hreichert
Copy link
Contributor Author

hreichert commented Sep 17, 2017

Default (current) colors:
chart5

Example with "500" colors from material.io with black theme on the openHAB android app:
screenshot_20170917-231907

New dark chart theme on the openHAB android app:
screenshot_20170917-231536

@hreichert hreichert changed the title [Charts] Enhancements to Charts: Themes, FontScale [Charts] Enhancements to Charts: Themes, FontScale, HideLegend Sep 17, 2017
hreichert added a commit to hreichert/smarthome that referenced this issue Sep 17, 2017
@kaikreuzer
Copy link
Contributor

Thanks for the good discussion - I specifically like the look of the dark theme for the Android app, this is indeed a huge improvement.

A few thoughts from my side:

For all ideas that are discussed, we have to keep in mind that they should be applicable to ANY chart provider; currently, we really only have one, but there might be others in future, which come with completely different styles and features.

The interface ChartProvider defines a parameter theme, but it's not used anywhere.

So it is indeed time to use it. It was @cdjackson who created that interface, so it might be a good idea to involve him in the discussion. The JavaDocs states: "The provider should store its own themes.". This seems to suggest that themes should be defined individually for a chart provider (within its own configuration). If we'd follow this approach, we would not have a ChartTheme class and no system wide defined themes at all.

What do you think about a "fontscale" parameter for the ChartServlet?

I would prefer to find a way to leave it to the chart provider to decide what is the most appropriate font size for the rendered image. The main issue currently is that the font size is chosen wrt the resolution of the image. With ever increasing dpis of our screens, this led to too small fonts now. So maybe a dpi setting might be a better choice than font-size? The chart provider could use this information to also optimize other parts like the line width, the grid, etc.

new parameter "hide-legend"
How shall we name it?

How about "legend=true/false" (i.e. requiring to specifically name it instead of using its presence to decide for the value)? We might actually want to use "true" as a default if the chart contains multiple lines and "false", if there is anyhow just a single value - this should probably address 95% of all cases without requiring the user to define that value.

@hreichert
Copy link
Contributor Author

Thanks @kaikreuzer for your suggestions, very appreciated.

The JavaDocs states: "The provider should store its own themes.". This seems to suggest that themes should be defined individually for a chart provider (within its own configuration). If we'd follow this approach, we would not have a ChartTheme class and no system wide defined themes at all.

Right, that's why I defined the interface in the package org.eclipse.smarthome.ui.internal.chart/defaultchartprovider. It's only meant to be used with the DefaultChartProvider. I see that this is not clear from my description above.
Every ChartProvider could use different features and styling possibilities, so it indeed makes no sense to define a superglobal interface.

I would prefer to find a way to leave it to the chart provider to decide what is the most appropriate font size for the rendered image. [...] So maybe a dpi setting might be a better choice than font-size?

Excellent. I was so locked in my "change the font SIZE" thoughts, that I completely ignored the existence of dpi.
Clearly dpi is a better solution and I will try to implement it.

How about "legend=true/false" (i.e. requiring to specifically name it instead of using its presence to decide for the value)?

For the ChartServlet it is/was hidelegend=true/false. Will change it to legend=true/false probably.

But for the sitemap (parameterless attribute hideLegend) I oriented on the switchSupport attribute from Slider:
http://docs.openhab.org/configuration/sitemaps.html#element-type-slider
Is a attribute X=true/false used anywhere in the sitemap?

We might actually want to use "true" as a default if the chart contains multiple lines and "false", if there is anyhow just a single value - this should probably address 95% of all cases without requiring the user to define that value.

👍

@mueller-ma
Copy link

@hreichert So you have the implementation for the Android App? Can you show me the code?

@hreichert
Copy link
Contributor Author

@mueller-ma No, I have done nothing in the app yet. For testing and screenshots I just temporarily modified the DEFAULT_THEME_NAME attribute in the DefaultChartProvider, so that I can see how it looks like in the app.

@hreichert
Copy link
Contributor Author

Thanks again @kaikreuzer for your DPI hint.
fontscale is gone - the chart provider now supports a dpi parameter that adjusts the font sizes, stroke width, grid line width, spacings, and so on. Albeit this needs a little more tweaking, now the charts already scale much better. Have a look at these images (not included here because some are hi-res):

300x200 96dpi:
https://user-images.githubusercontent.com/21249127/30571137-fb684ae0-9ce5-11e7-905e-13e6db76e389.png

3000x2000 450dpi:
https://user-images.githubusercontent.com/21249127/30571143-0a14c672-9ce6-11e7-9b67-20e6677d6862.png

6000x3000 680dpi:
https://user-images.githubusercontent.com/21249127/30571154-144f10d4-9ce6-11e7-8d13-8dcd9e5f9ffe.png

1920x1080 320 dpi (one of my smartphones):
https://user-images.githubusercontent.com/21249127/30571163-21a9c210-9ce6-11e7-89cb-a550a9fb79d2.png

@kaikreuzer
Copy link
Contributor

Is a attribute X=true/false used anywhere in the sitemap?

No, afaik not yet. But the issue with doing it similar to switchSupport is that we ALWAYS have a value - either true (if present) or false (if not). But we would actually have the "default" to decide automatically (based on the number of values on the chart). That's why we imho would need a "null" setting for "legend" and that would only work if we do a syntax like legend=true/false.

now the charts already scale much better

Looks pretty nice!

@hreichert
Copy link
Contributor Author

Indeed. Rework ongoing.

hreichert added a commit to hreichert/smarthome that referenced this issue Sep 19, 2017
hreichert added a commit to hreichert/smarthome that referenced this issue Sep 19, 2017
hreichert added a commit to hreichert/smarthome that referenced this issue Sep 21, 2017
@hreichert hreichert changed the title [Charts] Enhancements to Charts: Themes, FontScale, HideLegend [UI] Charts Enhancements: Themes, DPI, legend hiding Sep 21, 2017
hreichert added a commit to hreichert/smarthome that referenced this issue Sep 21, 2017
@hreichert
Copy link
Contributor Author

Done. Updated PR: #4291

kaikreuzer pushed a commit that referenced this issue Sep 25, 2017
Signed-off-by: Holger Reichert <mail@h0lger.de>
@hreichert
Copy link
Contributor Author

As the PR for this functionality is merged, I close this issue.
New features from the "Thinking further" section above get handled in other issues/repos.

@mueller-ma
Copy link

@hreichert You create a white theme with complete white background?

@hreichert
Copy link
Contributor Author

@mueller-ma Is the "bright" theme not sufficient?

@mueller-ma
Copy link

IMO it looks kind of outdated with the grey

@hreichert
Copy link
Contributor Author

hreichert commented Sep 25, 2017

What about this one?
chart_white

Compared to "bright":
chart_bright

I'm not happy with the yellow on both...

@mueller-ma
Copy link

Looks good

@mueller-ma
Copy link

You could remove it, 8 colors should be enough.
Do you use material colors on the default theme?

@hreichert
Copy link
Contributor Author

Replaced yellow with "lime" on the new white theme.
PR for new theme and other improvements: #4368

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants