BaseWidgetProvider Refactor #4640

ivorsmorenburg · 2024-09-15T14:53:55Z

Summary

Refactor of Widgets to improve the code and behaviour, abstracts widget logic and added repositories to add later on unit testing making the code more testable

Refactor BaseWdigetProvider & Added repositories
Improved offline widget interaction and error handling

Screenshots

Not Required

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#
#4601 (comment)

Any other notes

jpelgrom · 2024-09-15T16:28:37Z

Why are the widget repositories in common? There is absolutely no reason to have this around for watches.

You seem to be moving quite a lot of code around without making any changes to it (most notably: xml view attributes), which makes it harder to follow where there are 'real' changes. Is this really necessary, and if so could you at least split it up in a separate commit or something?

other loading improvements

Can you specify these 'improvements'? It is a refactor so you should try to limit functional changes.

ivorsmorenburg · 2024-09-15T16:54:47Z

Repositories can be move to a new location in a different PR together with all the current Widget Code is in common to a new place

dshokouhi · 2024-09-15T18:26:48Z

Added Frame to Camera Widget for better looking

if this is changing the design can you please provide a screenshot of the new look?

ivorsmorenburg · 2024-09-15T18:30:11Z

Added Frame to Camera Widget for better looking

if this is changing the design can you please provide a screenshot of the new look?
Also imaging the background in white for light theme

Fixes Static Widget Info wrongly using Button Widget Layout

…of the widgets Transparent background to camera.

dshokouhi · 2024-09-15T23:09:50Z

Added Frame to Camera Widget for better looking

if this is changing the design can you please provide a screenshot of the new look?
Also imaging the background in white for light theme

the transparent background was added a while ago for these widgets, I personally feel that it should become an option if we are going to add the background back. Tough to tell if the background is there when an actual image loads based on the provided picture. If we are going to do that we should really move it out of this PR.

This PR is starting to get a bit larger like the other one and that makes it more difficult to review. We appreciate the desire to fix everything but doing that in one PR is not easy to review. Smaller changes get reviewed easier and faster.

ivorsmorenburg · 2024-09-15T23:24:13Z

Added Frame to Camera Widget for better looking

if this is changing the design can you please provide a screenshot of the new look?
Also imaging the background in white for light theme

the transparent background was added a while ago for these widgets, I personally feel that it should become an option if we are going to add the background back. Tough to tell if the background is there when an actual image loads based on the provided picture. If we are going to do that we should really move it out of this PR.

This PR is starting to get a bit larger like the other one and that makes it more difficult to review. We appreciate the desire to fix everything but doing that in one PR is not easy to review. Smaller changes get reviewed easier and faster.

Just updated the description message as you referenced in that issue, the background frame for the camera has been removed. I feel this is the minimum changes to improve and refactor the widget side.

dshokouhi

There is a lot of changes in this PR so this review is really only a first pass after looking at the code briefly and running the debug APK for testing.

First it looks like the default widget theme was changed when I look at adding widgets in the launcher. The default theme for dynamic colors is now changed to the device theme which is odd and makes it less personable in my opinion.

Widget updates when teh screen turns on is not correct either. I am seeing production and the debug app falling out of sync when changes to the entity occur off screen. Production does stay up to as expected. I am also not seeing the subscription restarting to keep it up to date like production app.

Another request please go through the changes and any that show line numbers being changed without any real changes please revert them.

There is a lot and it makes teh review very difficult to follow and see if anything got lost as a result. I understand android studio may offer formatting but thats ok to use on new files, existing files its too much especially when other bits of code are touched it makes the review a lot harder.

app/src/main/java/io/homeassistant/companion/android/widgets/camera/CameraWidget.kt

app/src/main/java/io/homeassistant/companion/android/HomeAssistantApplication.kt

app/src/main/java/io/homeassistant/companion/android/widgets/camera/CameraWidget.kt

dshokouhi · 2024-09-16T00:55:24Z

.../java/io/homeassistant/companion/android/common/data/repositories/BaseDaoWidgetRepository.kt

@@ -0,0 +1,25 @@
+package io.homeassistant.companion.android.common.data.repositories


Please move this and all other files under the newly created repositories folder and place it under main to avoid adding this to Wear OS. The database code is fine to be here as both Wear OS and the main app share database code.

When adding repositories, will add a layer where we can do the logic to interact with the database (no Android objects). So wear should also use this methods rather than the database Dao directly, as this makes easier to make the logic testable.

home-assistant · 2024-09-16T00:57:39Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

ivorsmorenburg · 2024-09-16T11:43:58Z

This will be splitted into different PR's linked to the issues

Fixed camera duplicated Intent to UPDATE_IMAGE

dshokouhi

Looking at the changes proposed its still very difficult to review codewise and see what has changed and what makes sense. Can you please move out all the enhancements and bug fixes so we can keep this PR in the scope of widget refactor? That was the ask in the last PR to keep the refactor separated. Based on the proposed changes these changes are better reviewed outside of the refactor. The refactor itself has a lot going on, mixed with all the other enhancements its more difficult to tell what else is changed and impacted

Improved offline widget interaction and error handling
Recover from reboots and reinstalls using latest data and not showing "Problem Loading Widget"
Camera improvements when offline will keep the latest image fetched
Camera has a Icon to avoid seen an empty space on the screen for better UX until it gets the camera image
Very Small XML UI optimisations
Adds Missing CameraWidget to listen SCREEN Broadcast

dshokouhi

I havent not had a chance to do a code review but I did test the debug APK. I created a entity state widget to control the same light in both debug (left) and production (right) but if the state changes while the screen is off the debug build is not picking up the changes when the screen comes on. In fact I am seeing some errors that dont exist in the production build. For some reason the debug build refuses to update in this case until I toggle the entity in the widget. I would expect both widgets to update and always show the same state if changes happen when the screen is off. The widget should get a state update when the screen turns on in addition to subscribing for future updates. When you test this PR I recommend keeping both production and debug widgets side by side so you can make sure teh behavior works as expected.

2024-09-17 11:50:15.771 24001-24387 WebSocketRepository     io....stant.companion.android.debug  E  Websocket: onFailure (Ask Gemini)
                                                                                                    java.net.SocketException: Software caused connection abort
                                                                                                    	at java.net.SocketInputStream.socketRead0(Native Method)
                                                                                                    	at java.net.SocketInputStream.socketRead(SocketInputStream.java:118)
                                                                                                    	at java.net.SocketInputStream.read(SocketInputStream.java:173)
                                                                                                    	at java.net.SocketInputStream.read(SocketInputStream.java:143)
                                                                                                    	at com.android.org.conscrypt.ConscryptEngineSocket$SSLInputStream.readFromSocket(ConscryptEngineSocket.java:983)
                                                                                                    	at com.android.org.conscrypt.ConscryptEngineSocket$SSLInputStream.processDataFromSocket(ConscryptEngineSocket.java:947)
                                                                                                    	at com.android.org.conscrypt.ConscryptEngineSocket$SSLInputStream.readUntilDataAvailable(ConscryptEngineSocket.java:862)
                                                                                                    	at com.android.org.conscrypt.ConscryptEngineSocket$SSLInputStream.read(ConscryptEngineSocket.java:835)
                                                                                                    	at okio.InputStreamSource.read(JvmOkio.kt:93)
                                                                                                    	at okio.AsyncTimeout$source$1.read(AsyncTimeout.kt:153)
                                                                                                    	at okio.RealBufferedSource.request(RealBufferedSource.kt:210)
                                                                                                    	at okio.RealBufferedSource.require(RealBufferedSource.kt:203)
                                                                                                    	at okio.RealBufferedSource.readByte(RealBufferedSource.kt:213)
                                                                                                    	at okhttp3.internal.ws.WebSocketReader.readHeader(WebSocketReader.kt:124)
                                                                                                    	at okhttp3.internal.ws.WebSocketReader.processNextFrame(WebSocketReader.kt:107)
                                                                                                    	at okhttp3.internal.ws.RealWebSocket.loopReader(RealWebSocket.kt:307)
                                                                                                    	at okhttp3.internal.ws.RealWebSocket$connect$1.onResponse(RealWebSocket.kt:195)
                                                                                                    	at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:529)
                                                                                                    	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
                                                                                                    	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
                                                                                                    	at java.lang.Thread.run(Thread.java:1012)
2024-09-17 11:50:15.775 24001-24172 WebSocketRepository     io....stant.companion.android.debug  E  Exception while sending message (Ask Gemini)
                                                                                                    kotlinx.coroutines.TimeoutCancellationException: Timed out waiting for 30000 ms
                                                                                                    	at kotlinx.coroutines.TimeoutKt.TimeoutCancellationException(Timeout.kt:188)
                                                                                                    	at kotlinx.coroutines.TimeoutCoroutine.run(Timeout.kt:156)
                                                                                                    	at kotlinx.coroutines.EventLoopImplBase$DelayedRunnableTask.run(EventLoop.common.kt:505)
                                                                                                    	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:263)
                                                                                                    	at kotlinx.coroutines.DefaultExecutor.run(DefaultExecutor.kt:105)
                                                                                                    	at java.lang.Thread.run(Thread.java:1012)
2024-09-17 11:50:15.775 24001-24172 WebSocketRepository     io....stant.companion.android.debug  E  Issue re-registering subscription with {type=subscribe_trigger, trigger={platform=state, entity_id=[light.kitchen_lights_level]}}
2024-09-17 11:50:15.858 24001-24047 WebSocketRepository     io....stant.companion.android.debug  D  Unsubscribing from subscribe_trigger with data {trigger={platform=state, entity_id=[light.kitchen_lights_level]}}

oddly enough when it gets out of sync like this i no longer see the screen intents working in the debug build

2024-09-17 12:00:19.034 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Websocket: onOpen
2024-09-17 12:00:19.037 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Websocket: onMessage (text: {"type":"auth_required","ha_version":"2024.9.2"})
2024-09-17 12:00:19.040 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Message number null received
2024-09-17 12:00:19.043 24001-24394 WebSocketRepository     io....stant.companion.android.debug  D  Auth Requested
2024-09-17 12:00:19.051 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Websocket: onMessage (text: {"type":"auth_ok","ha_version":"2024.9.2"})
2024-09-17 12:00:19.053 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Message number null received
2024-09-17 12:00:19.056 24001-24045 WebSocketRepository     io....stant.companion.android.debug  D  Sending message 27: {type=supported_features, id=27, features={coalesce_messages=1}}
2024-09-17 12:00:19.058 24001-24045 WebSocketRepository     io....stant.companion.android.debug  D  Resubscribing to active subscriptions...
2024-09-17 12:00:19.060 24001-24045 WebSocketRepository     io....stant.companion.android.debug  D  Sending message 28: {type=subscribe_trigger, trigger={platform=state, entity_id=[light.kitchen_lights_level]}, id=28}
2024-09-17 12:00:19.062 24001-24045 WebSocketRepository     io....stant.companion.android.debug  D  Message number 28 sent
2024-09-17 12:00:19.069 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Websocket: onMessage (text: {"id":27,"type":"result","success":true,"result":null})
2024-09-17 12:00:19.071 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Message number 27 received
2024-09-17 12:00:19.079 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Websocket: onMessage (text: {"id":28,"type":"result","success":true,"result":null})
2024-09-17 12:00:19.081 24001-24387 WebSocketRepository     io....stant.companion.android.debug  D  Message number 28 received
2024-09-17 12:00:19.082 24001-24411 WebSocketRepository     io....stant.companion.android.debug  W  Response continuation has already been invoked for 28, null
2024-09-17 12:00:29.179 12473-12473 WebSocketRepository     io.homeassistant.companion.android   D  Sending message 36: {type=subscribe_trigger, trigger={platform=state, entity_id=[light.kitchen_lights_level]}, id=36}
2024-09-17 12:00:29.179 12473-12473 WebSocketRepository     io.homeassistant.companion.android   D  Message number 36 sent
2024-09-17 12:00:29.188 12473-15684 WebSocketRepository     io.homeassistant.companion.android   D  Websocket: onMessage (text)
2024-09-17 12:00:29.188 12473-15684 WebSocketRepository     io.homeassistant.companion.android   D  Message number 36 received
2024-09-17 12:00:29.228 12473-12521 WM-WorkerWrapper        io.homeassistant.companion.android   I  Worker result SUCCESS for Work [ id=023934eb-d3e4-42c2-9709-22943a18fd86, tags={ io.homeassistant.companion.android.websocket.WebsocketManager } ]
2024-09-17 12:00:59.183 12473-25015 WebSocketRepository     io.homeassistant.companion.android   D  Unsubscribing from subscribe_trigger with data {trigger={platform=state, entity_id=[light.kitchen_lights_level]}}
2024-09-17 12:00:59.183 12473-25015 WebSocketRepository     io.homeassistant.companion.android   D  Sending message 37: {type=unsubscribe_events, subscription=36, id=37}
2024-09-17 12:00:59.184 12473-25015 WebSocketRepository     io.homeassistant.companion.android   D  Message number 37 sent
2024-09-17 12:00:59.194 12473-15684 WebSocketRepository     io.homeassistant.companion.android   D  Websocket: onMessage (text)
2024-09-17 12:00:59.194 12473-15684 WebSocketRepository     io.homeassistant.companion.android   D  Message number 37 received
2024-09-17 12:00:59.239 12473-12521 WM-WorkerWrapper        io.homeassistant.companion.android   I  Worker result SUCCESS for Work [ id=58f0dc45-750f-41a4-820c-80e24c769cf5, tags={ io.homeassistant.companion.android.websocket.WebsocketManager } ]
2024-09-17 12:01:47.355 12473-12473 WebSocketRepository     io.homeassistant.companion.android   D  Sending message 38: {type=subscribe_trigger, trigger={platform=state, entity_id=[light.kitchen_lights_level]}, id=38}
2024-09-17 12:01:47.355 12473-12473 WebSocketRepository     io.homeassistant.companion.android   D  Message number 38 sent
2024-09-17 12:01:47.364 12473-15684 WebSocketRepository     io.homeassistant.companion.android   D  Websocket: onMessage (text)
2024-09-17 12:01:47.365 12473-15684 WebSocketRepository     io.homeassistant.companion.android   D  Message number 38 received
2024-09-17 12:01:47.476 12473-12521 WM-WorkerWrapper        io.homeassistant.companion.android   I  Worker result SUCCESS for Work [ id=0855d03b-ad43-42fb-939f-587e7389e91d, tags={ io.homeassistant.companion.android.websocket.WebsocketManager } ]

notice how the timestamps dont show anything for the debug build at the end? not sure where to pinpoint but should be easy to replicate and get the state out of sync.

dshokouhi · 2024-09-17T18:53:25Z

common/src/main/java/io/homeassistant/companion/android/database/widget/TemplateWidgetEntity.kt

@@ -10,6 +10,8 @@ data class TemplateWidgetEntity(
    override val id: Int,
    @ColumnInfo(name = "server_id", defaultValue = "0")
    override val serverId: Int,
+    @ColumnInfo(name = "entity_id", defaultValue = "template")
+    override val entityId: String,


why not make this nullable like you did in ButtonWidgetEntity.kt?

First refactor widgets

73946b0

home-assistant bot added the cla-signed label Sep 15, 2024

ivorsmorenburg added 3 commits September 15, 2024 15:54

Merge branch 'master' into widgetref2

cd3b1d2

Update Database Version

27a4997

Revert Accidental edit of schema

969f070

ivorsmorenburg added 2 commits September 15, 2024 23:20

Fixes home-assistant#4641 loading default look and feel

edfa4f6

Fixes Static Widget Info wrongly using Button Widget Layout

Fixes home-assistant#4641 loading default look and feel for the rest …

a3d8482

…of the widgets Transparent background to camera.

dshokouhi requested changes Sep 16, 2024

View reviewed changes

home-assistant bot marked this pull request as draft September 16, 2024 00:57

ivorsmorenburg closed this Sep 16, 2024

ivorsmorenburg deleted the widgetref2 branch September 16, 2024 08:16

This was referenced Sep 16, 2024

Media player has a big blue circle and is unresponsive. On reboots doesn't recover or control the linked media player. UI also looks odd #4646

Open

Improve placeholder for camera widget (keep previous when unable to load) #4644

Closed

ivorsmorenburg added 2 commits September 16, 2024 16:28

Addressed some comments

345bace

Addressed some comments

4ecf43d

Fixed camera duplicated Intent to UPDATE_IMAGE

ivorsmorenburg reopened this Sep 16, 2024

ivorsmorenburg added 2 commits September 16, 2024 19:05

Merge branch 'master' into widgetref2

f210184

Addressed some comments

84d2466

ivorsmorenburg marked this pull request as ready for review September 17, 2024 07:51

home-assistant bot requested a review from dshokouhi September 17, 2024 07:51

dshokouhi requested changes Sep 17, 2024

View reviewed changes

home-assistant bot marked this pull request as draft September 17, 2024 14:22

Merge branch 'home-assistant:master' into widgetref2

6da0943

This was referenced Sep 17, 2024

Adds missing CameraWidget registration for SCREEN_ON #4648

Open

Widget placeholder UI Improvements #4649

Merged

ivorsmorenburg force-pushed the widgetref2 branch from 5f986bf to 3abb1c2 Compare September 17, 2024 15:53

Addressed some comments

3705538

ivorsmorenburg force-pushed the widgetref2 branch from 3abb1c2 to 3705538 Compare September 17, 2024 17:35

ivorsmorenburg changed the title ~~First refactor for widgets~~ BaseWidgetProvider Refactor Sep 17, 2024

Merge branch 'master' into widgetref2

e317bc9

ivorsmorenburg requested a review from dshokouhi September 17, 2024 17:41

dshokouhi reviewed Sep 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BaseWidgetProvider Refactor #4640

BaseWidgetProvider Refactor #4640

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

jpelgrom commented Sep 15, 2024

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

dshokouhi commented Sep 15, 2024

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

dshokouhi commented Sep 15, 2024

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

dshokouhi left a comment

dshokouhi Sep 16, 2024

ivorsmorenburg Sep 16, 2024

home-assistant bot commented Sep 16, 2024

ivorsmorenburg commented Sep 16, 2024

dshokouhi left a comment

dshokouhi left a comment

dshokouhi Sep 17, 2024

		@@ -0,0 +1,25 @@
		package io.homeassistant.companion.android.common.data.repositories

BaseWidgetProvider Refactor #4640

Are you sure you want to change the base?

BaseWidgetProvider Refactor #4640

Conversation

ivorsmorenburg commented Sep 15, 2024 • edited Loading

Summary

Screenshots

Link to pull request in Documentation repository

Any other notes

jpelgrom commented Sep 15, 2024

ivorsmorenburg commented Sep 15, 2024 • edited Loading

dshokouhi commented Sep 15, 2024

ivorsmorenburg commented Sep 15, 2024 • edited Loading

dshokouhi commented Sep 15, 2024

ivorsmorenburg commented Sep 15, 2024 • edited Loading

dshokouhi left a comment

Choose a reason for hiding this comment

dshokouhi Sep 16, 2024

Choose a reason for hiding this comment

ivorsmorenburg Sep 16, 2024

Choose a reason for hiding this comment

home-assistant bot commented Sep 16, 2024

ivorsmorenburg commented Sep 16, 2024

dshokouhi left a comment

Choose a reason for hiding this comment

dshokouhi left a comment

Choose a reason for hiding this comment

dshokouhi Sep 17, 2024

Choose a reason for hiding this comment

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading

ivorsmorenburg commented Sep 15, 2024 •

edited

Loading