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

Graph menu proposal #3306

Closed
wants to merge 14 commits into from

Conversation

Philoul
Copy link
Contributor

@Philoul Philoul commented Apr 19, 2024

First commit to include a direct access to graph scale with long press
Second commit to keep menu opened during graph settings (to close menu just click outside menu

@olorinmaia
Copy link
Contributor

Built and tested ok :) Improves the functionality compared as it is today.

Is it possible, let's say you have 4 graphs, that when you make a change on graph 4 that position in menu is locked to where you did the change, now it jumps to top of menu and you need to scroll down again.

@Philoul
Copy link
Contributor Author

Philoul commented Apr 19, 2024

I noticed that when you are tuning third or fourth subgraph, but unfortunatly the menu is rebuilt on each click. Here I use existing features and I just show again menu to have it opened again (I don't think it's possible to save and restore shift position, and if yes I don't know how to do that easily...)

@Philoul Philoul marked this pull request as draft April 24, 2024 08:04
@Philoul
Copy link
Contributor Author

Philoul commented Apr 24, 2024

PR converted to draft because several variants of rescale graph are included at the same time for tests
On latest commit I added Text to rescale on simple click graph (selected scale in bigger size):
image

@vanelsberg
Copy link
Contributor

vanelsberg commented Apr 24, 2024

@Philoul Works as expected on current dev bits. Useful addition, thanks!

Just one remark: intuitively I'd expect clicking is used for rescaling, long-press for menu configuration. For my usage, for every say 99 in 100 actions it is to change scale, only 1 in 100 is used to actually change the graph.
Suggesting exchanging click and long-press?

I never used rescale on simple click graph. I find it unintuitive and somewhat unresponsive as on every rescale screen is re-rendering - especially when it is rotating to the desired scale. Scale menu (in the PR: long-press) on the other hand directly takes my to the expected view👍.
Suggesting disabling simple click graph?

@Philoul
Copy link
Contributor Author

Philoul commented Apr 24, 2024

@vanelsberg if we finally keep the4 textviews above graph to rescale, then we don't need anymore long click for rescale menu...

@vanelsberg
Copy link
Contributor

vanelsberg commented Apr 25, 2024

@vanelsberg if we finally keep the4 textviews above graph to rescale, then we don't need anymore long click for rescale menu...

Yes, Was following the discussion on Discord on this. My comment:
Actually, this is about AAPS overview which should be limited to showing ongoing status only? So imho important to keep away form "fancy views", limiting only to what is absolutely relevant...

@olorinmaia
Copy link
Contributor

olorinmaia commented Apr 27, 2024

@vanelsberg if we finally keep the4 textviews above graph to rescale, then we don't need anymore long click for rescale menu...

Yes, Was following the discussion on Discord on this. My comment: Actually, this is about AAPS overview which should be limited to showing ongoing status only? So imho important to keep away form "fancy views", limiting only to what is absolutely relevant...

I agree, the idea is good to simplify changing X hourly span of overview, but i think its sufficient to be able to change this by long pressing graph or the arrow button.

There is alot of information in the graphs, and we need to be careful to add more text to this without it being absolutely necessary

@Philoul
Copy link
Contributor Author

Philoul commented Apr 28, 2024

@olorinmaia @vanelsberg The idea was to reduce clicks for frequent actions and also to simplify graph menu very long.

So the 2 compromise can be compare to current solution (if no other ideas):

  1. with TextViews (or Buttons)
    ++ direct single click to rescale graph
    ++ graph menu simplification with no submenu for rescale
    ++ no long click action for graphs
    -- 4 additional items on graph area

  2. Long Press menu only
    ++ long click for graph rescale menu (better than 3 click but less than one single click action)
    -- most users will not use the long click feature on graph button (hidden so not intuitive) so will need 3 clicks (even the long click on graph not known by several users...)
    -- no simplification of the graph menu that should keep the rescale submenu

1 similar comment
@Philoul
Copy link
Contributor Author

Philoul commented Apr 29, 2024

@olorinmaia @vanelsberg The idea was to reduce clicks for frequent actions and also to simplify graph menu very long.

So the 2 compromise can be compare to current solution (if no other ideas):

  1. with TextViews (or Buttons)
    ++ direct single click to rescale graph
    ++ graph menu simplification with no submenu for rescale
    ++ no long click action for graphs
    -- 4 additional items on graph area

  2. Long Press menu only
    ++ long click for graph rescale menu (better than 3 click but less than one single click action)
    -- most users will not use the long click feature on graph button (hidden so not intuitive) so will need 3 clicks (even the long click on graph not known by several users...)
    -- no simplification of the graph menu that should keep the rescale submenu

@Philoul
Copy link
Contributor Author

Philoul commented May 12, 2024

See below a proposal to simplify graph menu.
In this proposal I deleted rescale submenu (available with Textview on the top left of main graph)
image

@vanelsberg
Copy link
Contributor

vanelsberg commented May 12, 2024

See below a proposal to simplify graph menu.

@Philoul
Much better, love this!

Request: could we merge this with PR#3311 (which has your "text menu" replaced with those nice scaling buttons?
(PS - Tried merging PR#3311 with yours but that resulted in a merge conflict...)

@Philoul
Copy link
Contributor Author

Philoul commented May 12, 2024

Request: could we merge this with PR#3311

This PR is only for testing, I asked @kenzo44 to open a dedicated PR to be able to test and compare both solutions for graph rescale. (that's why we have some little conflicts between both branches, but very easy to fix).

I think I will re-open a dedicated PR for final solution (I understood some users prefer my version with Textboxes overlapping main graph, and some other users prefer iceman's proposal with buttons above graph, both have ++ and --).

On final PR I will remove long press menu for rescale (I think now everyone prefer dedicated "buttons/textviews" solution), I got good comments on new menu for graph settings but would like to test it on low res screen to be sure it works fine (or tune it if necessary).

@MilosKozak any comments on this proposal (and variant with #3311)?

@vanelsberg
Copy link
Contributor

vanelsberg commented May 12, 2024

.. I think I will re-open a dedicated PR for final solution

@Philoul Sounds great. Would love that! Will test your new PR a.s.a.p.

On buttons vs text overlay: my original view was that the buttons would take up valuable screen resources 'without adding functionality'. But I changed my mind, mostly because in PR#3306 the text menu on the graph conflicts with the '*' icons showing up for profile changes. Relocating the text menu position would solved that, but could then overlay with the graph data itself which I see as a possible problem on graph readability?

@MilosKozak Other argument for using buttons (I think maybe): enhancing accessibility options reading the screen for users with a visual impairment?

@olorinmaia
Copy link
Contributor

I noticed that when you are tuning third or fourth subgraph, but unfortunatly the menu is rebuilt on each click. Here I use existing features and I just show again menu to have it opened again (I don't think it's possible to save and restore shift position, and if yes I don't know how to do that easily...)

I think you solved this very nicely @Philoul ! Love the new layout. Everything in one place without scrolling. I would also like to see how this looks with the buttons for 6, 12,18,24 hrs @kenzo44 have in his PR. Great job both of you as always =)

@Philoul
Copy link
Contributor Author

Philoul commented May 13, 2024

On latest commit pushed this evening I included several improvements, but I would like to have testers with very low res screen to test on physical device how this menu works...

As explain in a previous comment, this draft PR is for testing. I will probably close it to re do a new clean PR with final solution...

# Conflicts:
#	plugins/main/src/main/kotlin/app/aaps/plugins/main/general/overview/OverviewFragment.kt
#	plugins/main/src/main/kotlin/app/aaps/plugins/main/general/overview/OverviewMenusImpl.kt
#	plugins/main/src/main/res/layout/overview_graphs_layout.xml
@ga-zelle
Copy link

I prefer the text view alternative to save space. If it obscures anything just shortly change the scale and it won't be hidden any longer.

@robertrub
Copy link

robertrub commented May 16, 2024

I prefer the small texts without backgrounds and on the graph. Much less invasive that buttons. It'd be better for small screens too.!

Screenshot_20240516_161045_Firefox.jpg

@vanelsberg
Copy link
Contributor

vanelsberg commented May 16, 2024

Like the buttons, even when they take some screen space. Imho, textview impacts readability/accessibility. Rescaling to view is just a workaround then?

Edit: Also for me with buttons the UI looks a lot more cleaner and professional or may it is just that I hate using "text" areas as clickable item which UI-wise should be buttons? ;-)

Example with text menu:
xxx

Example with buttons:
yyyy

@robertrub
Copy link

Except the starts of the profile changes (that most people use rarely), there is no interesting information there.
Buttons would either use more height or cover more the graph :(

@vanelsberg
Copy link
Contributor

vanelsberg commented May 16, 2024

On using screen resources: graph scaling still is not optimal?
Most of the time I have 1 to 3 rows black graph space / empty rows at the top of the graph? It contains no graph data, only profile change icons?

@ga-zelle
Copy link

Alternatively the text views could be placed below the time axis labels. There is space left on top of graph 1 and they reside close to what they impact.

@robertrub
Copy link

Edit: Also for me with buttons the UI looks a lot more cleaner and professional or may it is just that I hate using "text" areas as clickable item which UI-wise should be buttons? ;-)

I agree with you but buttons use one more line and that is not great with small screens

Screenshot_20240516_182801_GitHub.jpg

@olorinmaia
Copy link
Contributor

olorinmaia commented May 17, 2024

I think both buttons and text is good solutions, there are + and - with both. It would be interesting to see how it looks if both of those are placed between main graph and graph 1.

@Philoul
Copy link
Contributor Author

Philoul commented May 18, 2024

3 variants of layout was tested:
On the left current version, in the middle, scale buttons moved below main graph (graph menu same place than now), on the left all buttons included above main graph.
To illustrate the overlay, I selected a scale (18h) with a lot of information...
image

With 6 hours scale and less visible information (much better)
image

@vanelsberg
Copy link
Contributor

Nice!

I'd go for the 3th option. There is enough "unused" space at the top of the graph.

Buttons do shield some of the profile indicators. Maybe (in another issue) move them slightly lower then?

If noth the 3th, then 2nd option (buttons below the graph) would also do for me.

@robertrub
Copy link

Here is my vote ;)

Screenshot_20240518_155158_Firefox.jpg

It think by aligning the buttons to the right they will cover less information and will stay "near" the area where they were before.

@vanelsberg
Copy link
Contributor

How about optionally hiding the buttons through the pulldown/config menu? (me, running for cover ;-) )

@Philoul
Copy link
Contributor Author

Philoul commented May 18, 2024

How about optionally hiding the buttons through the pulldown/config menu?

I during my several trials I put the 4 buttons vertically below menu icon. but it doesn't work... If you hide predictions, and/or if you select long range like 18h or 24h, you have too many recent information hidden by scale buttons...

@Philoul
Copy link
Contributor Author

Philoul commented May 18, 2024

How about optionally hiding the buttons through the pulldown/config menu?

And if your idea is to include these buttons within dropdown menu (in a first row of menu), keep in mind that this menu is hidden in simple mode (so only remaining way to resize scale in simple mode is long press on main graph...

@ga-zelle
Copy link

ga-zelle commented May 18, 2024

Leaning back and thinking about where we are now my first priority is to get rid io the long press changing time scale, second priority is not to waste srceen space. Anything else gets used very infrequently and therefore of minor importance with some pros and cons.

The current PR addresses two independent actions:

  1. selecting what to show in which graph. This PR has a nice solution for which there seems to be general agreement. It can be activated by the drop down button.
  2. selecting timescale could be done by the buttons which can be shown and activated by a long press on the graph. That is close to what the long press does now, i.e. easy to find. May be we need a 5th button "OK" to hide it again.

@vanelsberg
Copy link
Contributor

I during my several trials I put the 4 buttons vertically below menu icon. but it doesn't work...

Actually I was thinking more about only "showing" the scaling buttons from your screenshots above when selected, in a similar way the graphs are selected. So when not selected, buttons are not visible. But in that case we would need to keep the long-press to scale the graphs. Hmmm...

Maybe just ignore this idea. It is adding another complexity to a solution that is good enough...

Copy link

sonarcloud bot commented May 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
25 Security Hotspots
D Reliability Rating on New Code (required ≥ A)
C Security Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@Philoul
Copy link
Contributor Author

Philoul commented May 20, 2024

On last commit I replaced the 4 buttons by a popup rescale menu
image

Size and positionning of each graph /sub graph remains identical to current master
Overlay for rescale menu is less important than with 4 dedicated buttons
Rescale is done with 2 clicks (instead of 3 in current master, or 1 in previous proposal with 4 dedicated buttons)
Seems to be a good compromise for me, don't hesitate to test and comment here if you agree !

@vanelsberg
Copy link
Contributor

vanelsberg commented May 20, 2024

New PR with rescale menu tested ok. Nice! Think 2 clicks only is better than current dev.
(+ Grown accustomed 😢 to the 1-click rescaling buttons @robertrub voted for - can we please get them back😉)

@Philoul
Copy link
Contributor Author

Philoul commented May 24, 2024

Replaced by #3347

@Philoul Philoul closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants