[navigation]eta time switches with eta minutes on tap. #4741

rtsisyk · 2023-03-09T15:45:19Z

Aryan-Sagavekar commented

2023-03-09 15:45:19 +00:00

(Migrated from github.com)

Issue #4000
I also tried to add both the times together but it looked kind of messy being the same size, so it was better to switch them on tap.

Issue #4000 I also tried to add both the times together but it looked kind of messy being the same size, so it was better to switch them on tap. ![Screenshot_20230304-091010_Organic Maps (Debug)](https://user-images.githubusercontent.com/109228835/222874115-c19ff2b9-6688-403b-ac8e-b5b82549d244.jpg) ![Screenshot_20230304-091004_Organic Maps (Debug)](https://user-images.githubusercontent.com/109228835/222874124-87a8dd9f-34b7-4d3b-9d8b-48b184bc5c3c.jpg)

rtsisyk reviewed 2023-03-10 07:51:39 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -175,16 +189,40 @@ public class NavMenu

				    final long hours = TimeUnit.SECONDS.toHours(seconds);

				    final long minutes = TimeUnit.SECONDS.toMinutes(seconds) % 60;

rtsisyk commented

2023-03-10 07:51:39 +00:00

What does this code do?

Aryan-Sagavekar (Migrated from github.com) reviewed 2023-03-10 13:21:45 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -175,16 +189,40 @@ public class NavMenu

				    final long hours = TimeUnit.SECONDS.toHours(seconds);

				    final long minutes = TimeUnit.SECONDS.toMinutes(seconds) % 60;

Aryan-Sagavekar (Migrated from github.com) commented

2023-03-10 13:21:44 +00:00

it checks whether estimated min is displayed or eta time. Switches between the two on tap. People with weak eye sights would require both times in large font.

biodranik (Migrated from github.com) requested changes 2023-03-10 14:18:02 +00:00

biodranik (Migrated from github.com) left a comment

The previous implementation displayed two values at the same time. Now you made it worse, by displaying only one value and forcing users to tap in cases where they didn't need to tap.

The previous implementation displayed two values _at the same time_. Now you made it worse, by displaying _only one value_ and forcing users to tap in cases where they didn't need to tap.

biodranik (Migrated from github.com) requested changes 2023-03-18 19:32:07 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! It looks good, but an appropriate implementation is needed to avoid hard-coding and to work in all possible cases/layouts/sizes.

A help of an Android-guru is needed )

Thanks! It looks good, but an appropriate implementation is needed to avoid hard-coding and to work in all possible cases/layouts/sizes. A help of an Android-guru is needed )

biodranik (Migrated from github.com) reviewed 2023-03-25 13:14:34 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

biodranik (Migrated from github.com) commented

2023-03-25 13:11:16 +00:00

Why these changes are necessary?

android/src/app/organicmaps/widget/menu/NavMenu.java

biodranik (Migrated from github.com) commented

2023-03-25 13:11:00 +00:00

ditto

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

biodranik (Migrated from github.com) commented

2023-03-25 13:14:29 +00:00

I'm not convinced yet that all these calculations are necessary. We have two TextView elements, defined in the layout with custom colors already. What is necessary, is to replace the content in them (and make sure that updates to this content will go into the appropriate top or bottom element).

android/src/app/organicmaps/widget/menu/NavMenu.java

biodranik (Migrated from github.com) commented

2023-03-25 13:10:17 +00:00

Can't lambda be used here? like

v -> changeTimes()

Can't lambda be used here? like ``` v -> changeTimes() ```

Aryan-Sagavekar (Migrated from github.com) reviewed 2023-03-25 14:51:57 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

Aryan-Sagavekar (Migrated from github.com) commented

2023-03-25 14:51:57 +00:00

There are actually 4 textview elements in a linear layout(minutevalue minuteunit hour value hour unit) and 5th one is the eta(the one on the bottom by default). It made me uncomfortable too writing all those lines to change colour.
I'll try to find an alternative.

There are actually 4 textview elements in a linear layout(minutevalue minuteunit hour value hour unit) and 5th one is the eta(the one on the bottom by default). It made me uncomfortable too writing all those lines to change colour. I'll try to find an alternative.

biodranik (Migrated from github.com) reviewed 2023-03-25 15:27:01 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

biodranik (Migrated from github.com) commented

2023-03-25 15:27:01 +00:00

Can layouts be swapped somehow?

biodranik (Migrated from github.com) reviewed 2023-03-25 15:28:16 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

biodranik (Migrated from github.com) commented

2023-03-25 15:28:16 +00:00

Or maybe they can be defined (duplicated) in XML, and can be switched by the code?

Aryan-Sagavekar (Migrated from github.com) reviewed 2023-03-25 16:04:25 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

Aryan-Sagavekar (Migrated from github.com) commented

2023-03-25 16:04:24 +00:00

Yes that is a good idea.
We can set the alpha to 0 and switch it on tap?

Yes that is a good idea. We can set the alpha to 0 and switch it on tap?

biodranik (Migrated from github.com) reviewed 2023-03-25 16:20:55 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

biodranik (Migrated from github.com) commented

2023-03-25 16:20:54 +00:00

No alpha, please. Views should be made gone and visible.

No alpha, please. Views should be made `gone` and `visible`.

Aryan-Sagavekar (Migrated from github.com) reviewed 2023-03-25 16:24:04 +00:00

android/src/app/organicmaps/widget/menu/NavMenu.java

					
				@ -180,3 +196,4 @@

				    mTimeValueContainer.setOnClickListener(v -> changeTimes());

				    if (hours == 0)

				    {

				      UiUtils.hide(mTimeHourUnits, mTimeHourValue);

Aryan-Sagavekar (Migrated from github.com) commented

2023-03-25 16:24:03 +00:00

Okay. Understood

Jean-BaptisteC (Migrated from github.com) requested changes 2023-05-21 16:05:09 +00:00

Jean-BaptisteC (Migrated from github.com) left a comment

❌ Pixel 6 - Android 13
Hour is not centered

Tap to see time
Rotate screen -> switch is reset, OM shown minutes

❌ Pixel 6 - Android 13 Hour is not centered <img src="https://github.com/organicmaps/organicmaps/assets/87148630/2da9cc80-a669-4d3d-9af6-966569c01035" height=50/> - Tap to see time - Rotate screen -> switch is reset, OM shown minutes

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik

Jean-BaptisteC