Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix icon placement for section switches #3787

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

maniac103
Copy link
Contributor

@maniac103 maniac103 commented Jul 29, 2024

Fix issues with WidgetImageView skeleton placeholder

The 'automagic' skeleton addition caused two issues:

  • Misplaced icon in section switches ('Switch with mappings' icon misplaced #3773)
    This was caused by constraints in the widget's ConstraintLayout referencing the WidgetImageView's ID, which was no longer present after replacing it by the SkeletonLayout
  • Skeleton and image shown at the same time (mentioned in Icons misplaced #3786)
    This was caused by WidgetAdapter and SkeletonLayout both simultaneously modifying the visibility flag of the WidgetImageView, again caused by silent replacement of WidgetImageView by SkeletonLayout

Fix both issues by changing the approach: Instead of silently replacing the view, make WidgetImageView inherit from SkeletonLayout and make it redirect external calls to an internal ImageView instance.

Fixes #3773, #3786

@maniac103
Copy link
Contributor Author

This is due to Faltenreich/SkeletonLayout#47 ... I wonder whether we shouldn't think about a more generic solution though.

@maniac103
Copy link
Contributor Author

WidgetImageView will check that first before creating the skeleton: https://github.com/openhab/openhab-android/blob/main/mobile/src/main/java/org/openhab/habdroid/ui/widget/WidgetImageView.kt#L80-L85

Yes, but that solution is weird as well (views usually don't control their parent), and doesn't help with the fact that automagic skeleton creation is fundamentally broken (see #3786 (comment) and Faltenreich/SkeletonLayout#47). As far as I can tell, there are 3 possible solutions:

  • Add SkeletonLayout as parent to all WidgetImageViews, keep code as-is
  • Make WidgetImageView derive from SkeletonLayout and make it have an internal ImageView
  • Import the SkeletonMask related code into the app (it's not a large amount of code) and make WidgetImageView use it directly instead of using SkeletonLayout

I don't really like the first option (it's a lot of effort since we have a large number of layouts, and I still find it's somewhat of a hack as mentioned above), but I'm torn between the second and the third one currently.

The 'automagic' skeleton addition caused two issues:
- Misplaced icon in section switches (openhab#3773)
  This was caused by constraints in the widget's ConstraintLayout
  referencing the WidgetImageView's ID, which was no longer present
  after replacing it by the SkeletonLayout
- Skeleton and image shown at the same time (mentioned in openhab#3786)
  This was caused by WidgetAdapter and SkeletonLayout both
  simultaneously modifying the visibility flag of the WidgetImageView,
  again caused by silent replacement of WidgetImageView by
  SkeletonLayout

Fix both issues by changing the approach: Instead of silently replacing
the view, make WidgetImageView inherit from SkeletonLayout and make it
redirect external calls to an internal ImageView instance.

Fixes openhab#3773, openhab#3786

Signed-off-by: Danny Baumann <dannybaumann@web.de>
@maniac103 maniac103 force-pushed the sectionswitch-icon-fix branch from 1d1e203 to 201d492 Compare July 30, 2024 08:57
@maniac103
Copy link
Contributor Author

maniac103 commented Jul 30, 2024

Changed the approach and implemented option 2 (NB: for diff review, it's helpful to ignore whitespace changes in the diff viewer, since most part of the diff is reindention).

Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
@mueller-ma mueller-ma merged commit c83c5d4 into openhab:main Jul 30, 2024
8 checks passed
This was referenced Aug 14, 2024
maniac103 added a commit to maniac103/openhab.android that referenced this pull request Aug 14, 2024
This broke when WidgetImageView was changed to no longer be derived from
ImageView in openhab#3787. Since WidgetImageView passes all view attributes to
its internal ImageView instance, let the framework know that this is the
case.

Signed-off-by: Danny Baumann <dannybaumann@web.de>
maniac103 added a commit to maniac103/openhab.android that referenced this pull request Aug 16, 2024
This broke when WidgetImageView was changed to no longer be derived from
ImageView in openhab#3787. Since WidgetImageView passes all view attributes to
its internal ImageView instance, let the framework know that this is the
case.

Signed-off-by: Danny Baumann <dannybaumann@web.de>
maniac103 added a commit to maniac103/openhab.android that referenced this pull request Aug 16, 2024
This broke in openhab#3787

Fixes openhab#3802

Signed-off-by: Danny Baumann <dannybaumann@web.de>
mueller-ma pushed a commit that referenced this pull request Aug 16, 2024
This broke in #3787

Fixes #3802

Signed-off-by: Danny Baumann <dannybaumann@web.de>
mueller-ma pushed a commit that referenced this pull request Aug 19, 2024
This broke when WidgetImageView was changed to no longer be derived from
ImageView in #3787. Since WidgetImageView passes all view attributes to
its internal ImageView instance, let the framework know that this is the
case.

* Make WidgetImageView.InternalImageView.loadProgressCallback nullable

It's invoked via ImageView() -> setImageDrawable() ->
prepareForNonHttpImage(), at which time the initializer of
loadProgressCallback hasn't run yet.

Signed-off-by: Danny Baumann <dannybaumann@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Switch with mappings' icon misplaced
2 participants