Device specific actions could make the Card cumbersome to read (for developers) #1176

Open
opened 3 years ago by daniele · 9 comments
daniele commented 3 years ago
Owner

As discussed here:
https://github.com/Freeyourgadget/Gadgetbridge/issues/1165#issuecomment-408413216

the current procedure for adding a device specific action is:

This yields to a very long list of pictograms in the layout file, that might be confusing. Perhaps we might find a way to include sections of pictograms in each card, and to bind this new set of pictograms to device types.

As discussed here: https://github.com/Freeyourgadget/Gadgetbridge/issues/1165#issuecomment-408413216 the current procedure for adding a device specific action is: - adding it to the DeviceSupport class - add an icon to [the cards that appear in the control center](https://github.com/Freeyourgadget/Gadgetbridge/blob/master/app/src/main/res/layout/device_itemv2.xml) - toggle each icon visibility according to each device supported action [example](https://github.com/Freeyourgadget/Gadgetbridge/blob/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/adapter/GBDeviceAdapterv2.java#L156) This yields to a very long list of pictograms in the layout file, that might be confusing. Perhaps we might find a way to include sections of pictograms in each card, and to bind this new set of pictograms to device types.
Owner

(Why) do we need to define everything in the layout at all? Can't we just define the number of columns and the icon size and let the actual icons be defined somewhere else?

Then we could contribute the icons, together with the implementations from the outside, e.g. through the DeviceCoordinator. We can completely get rid of all action implementations and the device specific code in GBDeviceAdapterV2.

(Why) do we need to define everything in the layout at all? Can't we just define the number of columns and the icon size and let the actual icons be defined somewhere else? Then we could contribute the icons, together with the implementations from the outside, e.g. through the DeviceCoordinator. We can completely get rid of all action implementations and the device specific code in GBDeviceAdapterV2.
Poster
Owner

I think what you propose and other alternative approaches should be explored in branches / PRs, compared and the most satisfying approach merged to master.

If someone works on an implementation it would be good to mention this issue so that we can take a look.

I think what you propose and other alternative approaches should be explored in branches / PRs, compared and the most satisfying approach merged to master. If someone works on an implementation it would be good to mention this issue so that we can take a look.
Owner

While looking at this, I found out the following:
We use a RecyclerView in the ControlCenter. The documentation of RecyclerView says this:

If your app needs to display a scrolling list of elements based on large data sets (or data that frequently changes), you should use RecyclerView as described on this page.

We have neither a large data set (we usually have 1-5 elements (devices)), nor frequently changing elements.

RecyclerView is optimized for displaying many elements of the same type, while our use case is to display few elements of different type.

So I propose using a plain ListView instead of the RecyclerView, unless there are other reasons to use the RecyclerView. The ListView allows us to easily create device specific elements which custom actions.

While looking at this, I found out the following: We use a RecyclerView in the ControlCenter. The documentation of RecyclerView says this: > If your app needs to display a scrolling list of elements based on large data sets (or data that frequently changes), you should use RecyclerView as described on this page. We have neither a large data set (we usually have 1-5 elements (devices)), nor frequently changing elements. RecyclerView is optimized for displaying *many* elements of the same type, while our use case is to display few elements of different type. So I propose using a plain ListView instead of the RecyclerView, unless there are other reasons to use the RecyclerView. The ListView allows us to easily create device specific elements which custom actions.
Poster
Owner

I don't know if you can dynamically enlarge elements in a ListView, or if CardViews are allowed in a ListView.

Further I don't agree with your stated use case re the ".. elements of different type" part. The gadgets are different but we want to provide coherent means of using them (this was always my understanding at least) so how the elements are displayed in the list is important and it should be homogeneous IMHO.

I don't know if you can dynamically enlarge elements in a ListView, or if CardViews are allowed in a ListView. Further I don't agree with your stated use case re the ".. elements of **different type**" part. The gadgets are different but we want to provide coherent means of using them (this was always my understanding at least) so how the elements are displayed in the list is important and it should be homogeneous IMHO.
Owner

That needs to be checked indeed.

Note that I don't want to suddenly make all devices look different, far from that. The devices are different in the actions they support. With the current implementation, when I add support for a smart gardening device, then all cards will get the actions of the gardening device. And the gardening device will get the actions for alarms, "find device", etc., which just doesn't make sense.

The cards in general will continue to look the same, but they will only have those actions that the corresponding device actually supports.

That needs to be checked indeed. Note that I don't want to suddenly make all devices look different, far from that. The devices are different in the actions they support. With the current implementation, when I add support for a smart gardening device, then all cards will get the actions of the gardening device. And the gardening device will get the actions for alarms, "find device", etc., which just doesn't make sense. The cards in general will continue to look the same, but they will only have those actions that the corresponding device actually supports.
Poster
Owner

Note that I don't want to suddenly make all devices look different, far from that. The devices are different in the actions they support. With the current implementation, when I add support for a smart gardening device, then all cards will get the actions of the gardening device. And the gardening device will get the actions for alarms, "find device", etc., which just doesn't make sense.

This would happen anyway as long as we have a common Support class, but I might be missing something here.

The cards in general will continue to look the same, but they will only have those actions that the corresponding device actually supports.

I would very much prefer to have (for instance) the action-icon-row generated from the device specific support (or changed to be a collation of general and device-specific action) instead of reinventing everything starting from the container of the cards...

> Note that I don't want to suddenly make all devices look different, far from that. The devices are different in the actions they support. With the current implementation, when I add support for a smart gardening device, then all cards will get the actions of the gardening device. And the gardening device will get the actions for alarms, "find device", etc., which just doesn't make sense. This would happen anyway as long as we have a common Support class, but I might be missing something here. > The cards in general will continue to look the same, but they will only have those actions that the corresponding device actually supports. I would **very much** prefer to have (for instance) the action-icon-row generated from the device specific support (or changed to be a collation of general and device-specific action) instead of reinventing everything starting from the container of the cards...
Owner

Which Support class do you mean? DeviceSupport is just the backend -- I'm talking about the frontend and the visual representation.

I would very much prefer to have (for instance) the action-icon-row generated from the device specific support (or changed to be a collation of general and device-specific action) instead of reinventing everything starting from the container of the cards...

Well, this is exactly the problem. We can't do that, since all views are expected to be completely the same with RecyclerView.

Well, according to https://android.jlelse.eu/a-recyclerview-with-multiple-item-types-dfba3979050 it may be possible to have different item types, but that looks pretty complicated to me. No wonder, since RecyclerView is optimized for handling items of the same type.

Which Support class do you mean? `DeviceSupport` is just the backend -- I'm talking about the frontend and the visual representation. > I would **very much** prefer to have (for instance) the action-icon-row generated from the device specific support (or changed to be a collation of general and device-specific action) instead of reinventing everything starting from the container of the cards... Well, this is exactly the problem. We can't do that, since all views are expected to be completely the same with RecyclerView. Well, according to https://android.jlelse.eu/a-recyclerview-with-multiple-item-types-dfba3979050 it may be possible to have different item types, but that looks pretty complicated to me. No wonder, since RecyclerView is optimized for handling items of the same type.
Poster
Owner

Sorry I meant the coordinator.

As long as all device supported actions are made available through a single entity (which of course is part of the backend), then a single entity can manage the frontend in our case the DeviceAdapter.

The content of each card can already be different (and not only displayed or hidden). For instance we already using card-specific ItemWithDetailsAdapter in each card. I believe the same could happen for the action "icons row", without the need of refactoring the cards' container.

Sorry I meant the coordinator. As long as all device supported actions are made available through a single entity (which of course is part of the backend), then a single entity can manage the frontend in our case the [DeviceAdapter](https://github.com/Freeyourgadget/Gadgetbridge/blob/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/adapter/GBDeviceAdapterv2.java). The content of each card can already be different (and not only displayed or hidden). For instance we already using card-specific ``ItemWithDetailsAdapter`` in each card. I believe the same could happen for the action "icons row", without the need of refactoring the cards' container.
Owner

We have device specific coordinator classes/instances, device specific support classes/instances and just as well we should have device specific frontend classes/instances.

I can't see how this could currently be possible. We have a single GBDeviceAdapterv2 class and instance and a single ViewHolder class, which does everything for all devices. Can we have device specific ViewHolder classes and instances? AFAIU, this is the difficult part with RecyclerView.

We have device specific coordinator classes/instances, device specific support classes/instances and just as well we should have device specific frontend classes/instances. I can't see how this could currently be possible. We have a single `GBDeviceAdapterv2` class and instance and a single `ViewHolder` class, which does everything for all devices. Can we have device specific `ViewHolder` classes and instances? AFAIU, this is the difficult part with RecyclerView.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.