StephenBlackWasAlreadyTaken / xDrip-Experimental

Experimental Branches for Collaboration on DexDrip
GNU General Public License v3.0
25 stars 62 forks source link

Extra status line #257

Closed tzachi-dar closed 8 years ago

tzachi-dar commented 8 years ago

This is a very first version of this pr, looking for more feedback.

Here is what it does:

Add support for 7" tablet. Add extra status line. (need to be configured from the UI). Add clock for large and extra large tablet.

Still needs some more playing with font sizes and maybe other changes to the UI.

Any feedback is welcomed.

AdrianLxM commented 8 years ago

Hi, do you have a screenshot?

Before finally merging it - will there be a setting for the status line?

Proguard I'd like to have in a separate PR. When we decide to merge it I'd like to change the "get it all" rules for more specific rules in order for it to warn on future problems.

tzachi-dar commented 8 years ago

I'll do more fine tuning and send the screen shots on 3 sizes... yes, there will be setting for the status line. I guess most people will not use it. I'll only added proguard here to allow me to easier test it. I'll remove it before the final checkin.

tzachi-dar commented 8 years ago

Here is how things look on a 7" tablet: screenshot_2016-01-21-00-45-33 screenshot_2016-01-21-00-46-07

tzachi-dar commented 8 years ago

Here are two more screenshots from a 5" screen: screenshot_2016-01-21-01-02-20 screenshot_2016-01-21-01-03-40

tzachi-dar commented 8 years ago

All comments are welcomed. I might have to reduce the chart size to allow the extra line of text.

jstevensog commented 8 years ago

Hi Tzachi-dar, How does it look when used with xBridge, and the bridge battery is displayed? Cheers

On Thu, Jan 21, 2016 at 10:09 AM, tzachi-dar notifications@github.com wrote:

All comments are welcomed. I might have to reduce the chart size to allow the extra line of text.

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173394460 .

John Stevens "You are how you live, not what you have."

tzachi-dar commented 8 years ago

בתאריך 21 בינו 2016 02:45,‏ "John" notifications@github.com כתב:

Hi Tzachi-dar, How does it look when used with xBridge, and the bridge battery is displayed? Cheers

On Thu, Jan 21, 2016 at 10:09 AM, tzachi-dar notifications@github.com wrote:

All comments are welcomed. I might have to reduce the chart size to allow the extra line of text.

— Reply to this email directly or view it on GitHub < https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173394460

.

John Stevens "You are how you live, not what you have."

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173414578 .

AdrianLxM commented 8 years ago

Hi,

first of all: As long as it is extra information that you have to enable via the settings I'm fine with it.

Just a few more thoughts:

  1. Can we have it a bit more compact? That would be great for smaller devices slope = 1,18 inter = -42,89 -> s:1,18|i:-42,89
  2. It doesn't change that often - actually just with a new calibration where interested people can check the calibration graph or datatable afterwards. It might be interesting for the viewer mode though. There are 2 things that change more frequently and may be more interesting:
    • _The time_ - especially for small devices that don't show the time in the status.
    • _filtered vs unfiltered_
    • an excerpt of todays statistics
    • For later on: some indication of noise
    • ...

-> I'd like to add an extra settings page where people can choose what they want. But I can do this in a later PR.

tzachi-dar commented 8 years ago

As for small screens, I'll change that to have a smaller text. As for time, I do add the time for the big tablets, can add that for small device as well, but do they have screen space?

As for importance of this information. Well generally, we do bg = slope * raw +inter.

I have seen places where inter was 70. Seen even one time that it was 200. This means that hypo will happen and xDrip will show bad values. I intend to fix this, but fixing the algorithm will take time, and is much harder.

I hope that people will look at it and be more careful when intercept is a big positive number.

By the way, after a calibration the values change in two steps, so one has to remember to look at the calibration graph.

Thanks Tzachi

On Thu, Jan 21, 2016 at 10:26 AM, AdrianLxM notifications@github.com wrote:

Hi,

first of all: As long as it is extra information that you have to enable via the settings I'm fine with it.

Just a few more thoughts:

1.

Can we have it a bit more compact? That would be great for smaller devices slope = 1,18 inter = -42,89 -> s:1,18|i:-42,89 2.

It doesn't change that often - actually just with a new calibration where interested people can check the calibration graph or datatable afterwards. It might be interesting for the viewer mode though. There are 2 things that change more frequently and may be more interesting:

  • The time - especially for small devices that don't show the time in the status.
    • filtered vs unfiltered
    • an excerpt of todays statistics
    • For later on: some indication of noise
    • ...

-> I'd like to add an extra settings page where people can choose what they want. But I can do this in a later PR.

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173494542 .

AdrianLxM commented 8 years ago

The text size for smaller devices is fine. It will automatically be smaller.

Please tell me when you have finished, I will add the other features plus a settings page then.

tzachi-dar commented 8 years ago

Not sure that I agree about text size. I have to manually increase it for bigger devices. Are you sure that I don't have to do that also for smaller devices (the smallest device that I have is 4.5").

AdrianLxM commented 8 years ago

I have one of those Phone-Watches with 1.5 inches. A smaller text size would be unreadable. Here is a photo (the only one I've got with me atm; not that good quality but you can see the text size): http://up.picr.de/24033276pk.jpg

AdrianLxM commented 8 years ago

setTextSize(float size) will set the text in scaled pixels.

    @android.view.RemotableViewMethod
    public void setTextSize(float size) {
        setTextSize(TypedValue.COMPLEX_UNIT_SP, size);
    }

So making it larger for larger tablet does make sense as it should keep the proportion and be readable from further apart.

On smaller devices the scaled pixels seem to be a bit smaller already but making it even smaller (= keeping the proportion) would make it unreadable. You don't read a watch-phone from a closer distance than a regular size mobile phone. On very small devices the proportions are already off but they are quite readable and even the stats are usable (what really surprised me).

tzachi-dar commented 8 years ago

OK, I have finished what I wanted to do. All comments are welcomed. Adrian, you are welcomed to test this on small screens and do the changes that you want.

2 things to note: I have given the extra status line a text view of itself so it can have it's own colors in the future. I have also decreased the graph size on the different displays so the extra line will not cover it.

AdrianLxM commented 8 years ago

I'm sorry, I will need a couple of days for this.

I'm planning to add: time, mean-BG, In-range-%, high-range%, low-range%, A1c and time since last calibration. (All statistic values are of today) The user can then mark the one he wants. All won't fit but the user can decide what is important. The default selection would be the calibration data.

@tzachi-dar do you want to undo the proguard-commits first?

tzachi-dar commented 8 years ago

Take your time. Once your changes are ready, we will cherry-pick all changes and put them above the master, this will allow us to test the proguard, but not to have it in when merged. As for all the fields you want to add, will we calculate them each time or do you plan on something smarter?

AdrianLxM commented 8 years ago

Regarding the new line and "Min ago": Well, when looking at the layout file it looks like the extra status line is always below "notices" and therefore below the "Minutes ago" line.

Regarding calculation: Without creating objects the db-queries count and avg on just today's values are quite fast. What makes it slow is the query for the chart that really creates objects.

And another thing I've noticed: you have introduced a margin of 165 for the chart if no extra line is shown. Why is this?

Have a good night.

tzachi-dar commented 8 years ago

As for the "min ago". This is needed at least for the large tablets since I have increased the fonts there.

As for calculations. If they are fast, than that is fine. Obviously averages can be calculated without going over all points, but if things are fast, then let's keep it simple...

Screen proportions are obviously the most complex thing here... so, 165 is the value that exists on my nexus5. I really did not want to change anything there so at first I wanted to do something like:

else { ViewGroup.MarginLayoutParams params = chart.getLayoutParams(); if(displayExtraLine) { params.topMargin += 35; } chart.setLayoutParams(params); }

but that kept growing every time the screen has moved.

So, perhaps a better solution will be something like static initial_top_margin = -1; if (initial_top_margin = -1) { ViewGroup.MarginLayoutParams params = chart.getLayoutParams(); initial_top_margin = params.topMargin; }

And than we will use the initial_top_margin when needed. This is a better solution since it will not cause changes to existing devices.

If you want, I can do that, or you can do that as part of your changes.

Thanks Tzachi

On Fri, Jan 22, 2016 at 2:33 AM, AdrianLxM notifications@github.com wrote:

Regarding the new line and "Min ago": Well, when looking at the layout file it looks like the extra status line is always below "notices" and therefore below the "Minutes ago" line.

Regarding calculation: Without creating objects the db-queries count and avg on just today's values are quite fast. What makes it slow is the query for the chart that really creates objects.

And another thing I've noticed: you have introduced a margin of 165 for the chart if no extra line is shown. Why is this?

Have a good night.

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173761186 .

AdrianLxM commented 8 years ago

@tzachi-dar, as you have devices that fall under the category largeTablet and xLargeTablet it would be best if you could do that.

I'm just not sure if a static variable is the preferred way for that. It will work as the lifetime is longer than the activity (and is anyone really switching displays while android is running?) but... Usually Android has the appropriate callbacks to handle the life-cyle of an activity. Do we really need to set the margin every time a new value comes in? Wouldn't it suffice to just set the margin once in onCreate or onResume or onStart? Another possibility could be to use ViewTreeObserver.OnGlobalLayoutListener. and check for layout changes.

But I don't have time for this and don't expect anyone else to do this either if a static variable works.

I would just prefer a boolean flag like topMarginCalculated over the special value -1 as margins can actually be negative.

AdrianLxM commented 8 years ago

And now I feel quite stupid (not good writing things in a hurry during lunch break):

Why not ad two more layouts for large and extra-large tablets where the margin is set? Like the image resources we can have in different resolutions, we can have one default layout in res/layout/ and different layouts for different sizes in res/layout-xlarge/ e.g. (or as we develop for Android versions above 3.2 it is even possible to specify the needed screen width like res/layout-w1024dp/ or height). http://developer.android.com/guide/practices/screens_support.html#DeclaringTabletLayouts

The chart-view then could be declared to start below the layout holding the status lines with a certain margin for each screen size. It then should automatically adopt when calling extraStatusLineText.setVisibility(View.GONE);

tzachi-dar commented 8 years ago

Hi, as for using differenet layouts for different sizes...

Well, my first implmentation was using this method. So, why did I remove it? Since the different layouts are not that different, it just failt that I took one file, copied it, changed one line and than every time a new change was introduced to the first file, things did not work until I have copied them... I believe that if we will really have a different UI for different tablets than multiple files are the way to go.

So, I decided that if the change is realy only in a very few places, doing that in code would be more cleaver.

I'll move the initialization of things to on create and on resume to make it more correct. Hope to have time for that tommorow night.

Thanks Tzachi

On Fri, Jan 22, 2016 at 2:00 PM, AdrianLxM notifications@github.com wrote:

And now I feel quite stupid (not good writing things in a hurry during lunch break):

Why not adding two more layouts for large and extra-large tablets where the margin is set? Like the image resources we can have in different resolutions, we can have one default layout in res/layout/ and different layouts for different sizes in res/layout-xlarge/ e.g. (or as we develop for Android versions above 3.2 it is even possible to specify the needed screen width like res/layout-w1024dp/ or height).

http://developer.android.com/guide/practices/screens_support.html#DeclaringTabletLayouts

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173899745 .

AdrianLxM commented 8 years ago

Well, no matter if we will have different Layouts or not: Defining the chart view like this:

                <lecho.lib.hellocharts.view.LineChartView
                    android:id="@+id/chart"
                    android:layout_width="match_parent"
                    android:layout_alignParentStart="false"
                    android:layout_alignParentEnd="true"
                    android:layout_height="match_parent"
                    android:layout_below="@+id/extraStatusLine"/>

... should make the the whole conditional (depending on the presence of the status line) addition of margins in the code unnecessary.

tzachi-dar commented 8 years ago

Can you add this to the branch, so I can try this? בתאריך 22 בינו 2016 14:47,‏ "AdrianLxM" notifications@github.com כתב:

Well, no matter if we will have different Layouts or not: Defining the chart view like this:

            <lecho.lib.hellocharts.view.LineChartView
                android:id="@+id/chart"
                android:layout_width="match_parent"
                android:layout_alignParentStart="false"
                android:layout_alignParentEnd="true"
                android:layout_height="match_parent"
                android:layout_below="@+id/extraStatusLine"/>

... should make the the whole conditional (depending on the presence of the status line) addition of margins in the code unnecessary.

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173911096 .

AdrianLxM commented 8 years ago

Here is a pull request pointed at your branch (not master): https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/258

tzachi-dar commented 8 years ago

Thanks, will try it tommorow. בתאריך 22 בינו 2016 15:35,‏ "AdrianLxM" notifications@github.com כתב:

Here is a pull request pointed at your branch (not master): #258 https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/258

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-173923079 .

AdrianLxM commented 8 years ago

Hi, ok, in the mean time I have added the preference page. The default is just the long version of the calibration data.

Settings: Settings1 Settings2

Full Status Line (just for demonstration purposes): Full line A Selection of items: Selection

No Status Line (looks like before): No Status Line

It also looks good with xBridge-Battery (no screenshot :( )

AdrianLxM commented 8 years ago

I've added the stats for "today" to the pr: https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/258

screenshot_2016-01-23-20-04-03 (Text size on the phone set to large.)

From my side I'm finished with the settings and features I wanted to add. The layout works fine for me on my devices without the need of conditional margins.

tzachi-dar commented 8 years ago

OK, this is my latest version. All comments are welcomed. I'll continue checking this...

AdrianLxM commented 8 years ago

If you could remove the proguard changes please, it looks mergeable to me :+1:

tzachi-dar commented 8 years ago

Sure, i'll make some more tests tonight and if all is well i'll merge it. בתאריך 24 בינו 2016 16:33,‏ "AdrianLxM" notifications@github.com כתב:

If you could remove the proguard changes please, it looks mergeable to me [image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/257#issuecomment-174303616 .

AdrianLxM commented 8 years ago

Closing this as it is already in master from https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/260