Implement widget for the current speed #9738

AndrewShkrob · 2024-11-24T22:49:44Z

TobiPeterG commented

2024-11-24 22:49:44 +00:00

(Migrated from github.com)

This is a first draft to add a "widget" to show the current speed. It connects to the current speed limit and allows us to remove the speed from the bottom bar entirely. I'm completely happy with the current design, as it is quite distached from other UI elements at the moment. This is due to the next-turn-image that is sometimes visible and sometimes not. Maybe I should place it further down?

Also, the position in landscape mode is not optimal next to the lanes, it looks quite cluttered. It could be moved to the left side as well, but there it is blocked by the bottom menu (could be moved to the right) and two other buttons are also on that side interfering (could also be moved to the right side).

What are the thoughts about this?
The speed limit sign and connecting background appears/disappears with an animation.

Video:

https://github.com/user-attachments/assets/db60a02c-b2a9-4843-b25b-5ad1d385e44b

Depends on https://git.omaps.dev/organicmaps/organicmaps/pulls/9355 This is a first draft to add a "widget" to show the current speed. It connects to the current speed limit and allows us to remove the speed from the bottom bar entirely. I'm completely happy with the current design, as it is quite distached from other UI elements at the moment. This is due to the next-turn-image that is sometimes visible and sometimes not. Maybe I should place it further down? Also, the position in landscape mode is not optimal next to the lanes, it looks quite cluttered. It could be moved to the left side as well, but there it is blocked by the bottom menu (could be moved to the right) and two other buttons are also on that side interfering (could also be moved to the right side). What are the thoughts about this? The speed limit sign and connecting background appears/disappears with an animation. Video: https://github.com/user-attachments/assets/db60a02c-b2a9-4843-b25b-5ad1d385e44b

👍 2

AndrewShkrob reviewed 2024-12-21 22:04:53 +00:00

android/app/src/main/res/values/attrs.xml

					
				@ -2,7 +2,9 @@

				<resources>

				  <declare-styleable name="SpeedLimitView">

				    <attr name="BackgroundColor" format="color" />

				    <attr name="WidgetBackgroundColor" format="color" />

AndrewShkrob commented

2024-12-21 22:04:52 +00:00

What's the difference between BackgroundColor and WidgetBackgroundColor?
It should be possible to understand it from the name.
Btw, why do these values have names starting from upper case and values below have names starting from lower case?
They all should start from lower case letters like any other android's XML value.

What's the difference between `BackgroundColor` and `WidgetBackgroundColor`? It should be possible to understand it from the name. Btw, why do these values have names starting from upper case and values below have names starting from lower case? They all should start from lower case letters like any other android's XML value.

AndrewShkrob reviewed 2024-12-21 22:05:34 +00:00

AndrewShkrob reviewed 2024-12-21 22:09:08 +00:00

android/app/src/main/java/app/organicmaps/widget/SpeedLimitView.java

					
				@ -172,32 +390,106 @@ public class SpeedLimitView extends View

				    canvas.drawText(mSpeedLimitStr, cx, textY, mTextPaint);

				  }

AndrewShkrob commented

2024-12-21 22:09:08 +00:00

  private void drawWidgets(@NonNull Canvas canvas, float cx, float cx_or_cy, float cy, boolean isLandscape) {

```suggestion private void drawWidgets(@NonNull Canvas canvas, float cx, float cx_or_cy, float cy, boolean isLandscape) { ```

AndrewShkrob reviewed 2024-12-21 22:18:47 +00:00

android/app/src/main/java/app/organicmaps/widget/SpeedLimitView.java

AndrewShkrob commented

2024-12-21 22:18:47 +00:00

What's wrong with Math.pow()? Does your solution work faster?

What's wrong with `Math.pow()`? Does your solution work faster?

AndrewShkrob reviewed 2024-12-21 22:27:41 +00:00

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

AndrewShkrob commented

2024-12-21 22:27:41 +00:00

Nobody approved this removal yet :)
I think it's better to make it in separate PR

Nobody approved this removal yet :) I think it's better to make it in separate PR

AndrewShkrob requested changes 2024-12-22 09:21:54 +00:00

AndrewShkrob left a comment

Unfortunately, there are a lot of things I don't like in this PR :(

As I already told you before, please format the code with our formatter (and don't forget about empty lines between methods): organicmaps/organicmaps#9355 (comment)
I don't see the point in performing a code review until the main architectural issue is resolved.

Let's discuss the 2nd point.
Imagine you're a developer that sees this code for the first time. We have a class SpeedLimitView. What do you think this class does?
You would probably say something like:

Hey, the class name has a SpeedLimit and a View. I would assume it draws a speed limit sign on the screen.

Well, you would be surprised when you discover that instead of a simple speed limit sign it also draws a current speed, unlimited sign, a background to combine all of these signs together and it does much more stuff under the hood.

Let's try to split the code into separate classes to make it maintainable and easily extendable in the future.

How to do this?
The reason why you decided to write all the code inside the one class is because the code is pretty similar however it is a different functionality.
So, let's move all the similar code into a base class (f.e. SignView).
Then for each specific task (in our case for each specific sign) we will have a separate class - SpeedLimitView, CurrentSpeedView, UnlimitedSignView.
This way it would be easily maintainable because each class will have a lot less lines of code and it would be easily extendable f.e. when we need to draw another sign we'll just create another class instead of diving inside a class with over 1k lines of code trying to find a place where to add our new code and later spending week in testing that we didn't break anything :)

I don't like talking about OOP and patterns and all this stuff but looks like it's a good time :)
Please, check SOLID principles. SRP and OCP are exactly what your code needs right now.

That was the first main issue. The second one is how to combine all these signs.
We simply need a container class here.
I have some thoughts:

Have you considered using LinearLayout for this? You can configure background and animations for it in XML. No need to write the code especially for such simple things.
If this doesn't work then you can consider extending LinearLayout class with your own functionality.
If this also doesn't work then (and only then) we can think about writing our custom container class that will hold and place our sign views in the correct places.

Very important note.
We're discussing here UI classes. They MUST behave like other Android's UI elements. All these classes (SpeedLimitView, CurrentSpeedView, UnlimitedSignView, SignContainer) must work like default android views. You should be able to configure them in layout inspector, to see their dimensions/sizes, where they are placed, how they behave with different parameters and so on. And, of course,
you must be able to see them rendered in the layout inspector.

After these issues are resolved, we can proceed with the code review. Until then, sorry, I can't approve it.

Unfortunately, there are a lot of things I don't like in this PR :( 1. As I already told you before, please format the code with our formatter (and don't forget about empty lines between methods): https://git.omaps.dev/organicmaps/organicmaps/pulls/9355#issuecomment-2370809004 2. I don't see the point in performing a code review until the main architectural issue is resolved. Let's discuss the 2nd point. Imagine you're a developer that sees this code for the first time. We have a class `SpeedLimitView`. What do you think this class does? You would probably say something like: - Hey, the class name has a `SpeedLimit` and a `View`. I would assume it draws a speed limit sign on the screen. Well, you would be surprised when you discover that instead of a simple speed limit sign it also draws a current speed, unlimited sign, a background to combine all of these signs together and it does much more stuff under the hood. Let's try to split the code into separate classes to make it maintainable and easily extendable in the future. How to do this? The reason why you decided to write all the code inside the one class is because the code is pretty similar however it is a different functionality. So, let's move all the similar code into a base class (f.e. `SignView`). Then for each specific task (in our case for each specific sign) we will have a separate class - `SpeedLimitView`, `CurrentSpeedView`, `UnlimitedSignView`. This way it would be easily maintainable because each class will have a lot less lines of code and it would be easily extendable f.e. when we need to draw another sign we'll just create another class instead of diving inside a class with over 1k lines of code trying to find a place where to add our new code and later spending week in testing that we didn't break anything :) I don't like talking about OOP and patterns and all this stuff but looks like it's a good time :) Please, check SOLID principles. SRP and OCP are exactly what your code needs right now. That was the first main issue. The second one is how to combine all these signs. We simply need a container class here. I have some thoughts: 1. Have you considered using LinearLayout for this? You can configure background and animations for it in XML. No need to write the code especially for such simple things. 2. If this doesn't work then you can consider extending LinearLayout class with your own functionality. 3. If this also doesn't work then (and only then) we can think about writing our custom container class that will hold and place our sign views in the correct places. Very important note. We're discussing here UI classes. They MUST behave like other Android's UI elements. All these classes (SpeedLimitView, CurrentSpeedView, UnlimitedSignView, SignContainer) must work like default android views. You should be able to configure them in layout inspector, to see their dimensions/sizes, where they are placed, how they behave with different parameters and so on. And, of course, you must be able to see them rendered in the layout inspector. After these issues are resolved, we can proceed with the code review. Until then, sorry, I can't approve it.

gpesquero reviewed 2024-12-28 13:43:26 +00:00

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

gpesquero commented

2024-12-28 13:43:26 +00:00

Nobody approved this removal yet :)
I think it's better to make it in separate PR

@AndrewShkrob, @TobiPeterG: I've launched PR #9955 for the removal of the speed display in the navigation panel, so we can discuss there the rearrange of the items in the navigation panel while the new Speed widget is implemented.

> Nobody approved this removal yet :) > I think it's better to make it in separate PR @AndrewShkrob, @TobiPeterG: I've launched [PR #9955](https://git.omaps.dev/organicmaps/organicmaps/pulls/9955) for the removal of the speed display in the navigation panel, so we can discuss there the rearrange of the items in the navigation panel while the new Speed widget is implemented.

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

No reviewers

AndrewShkrob