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

feat: add hot network questions widget #144

Merged
merged 26 commits into from
Jul 15, 2023
Merged

feat: add hot network questions widget #144

merged 26 commits into from
Jul 15, 2023

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jun 25, 2023

Description

This implements the widget aspect of #142, which I realised is the main thing that is important to me - I'm happy to edit that issue to explicitly remove the ask for a view, but figure it's not a big deal as this still the meat of what's needed for that too.

Overall this is pretty complete, there's just a couple of minor things I need to address - specifically:

  1. ensuring the widget can recover from a bad state, especially as I think Android kills it for taking too long to load initially
    • I think this can partly be improved by ensuring the refresh onclick handler is registered asap, but also know this is because I'm probably incorrectly doing async calls i.e. I should be deferring the load of the hot questions to the background or something...
  2. its possible to get the widget/app into a state where it doesn't open the question; I'm pretty sure this is related to create vs resume logic, because when it happens the app doesn't disappear the question details content
    • I think I know how to fix this as I spent an annoying amount of time initially figuring out how to get the app to open specific questions (namely I was having trouble with the intent extras getting passed around, until I discovered I had to give a unique request identifier and action) but have not had the time to try it out yet
  3. misc. UI improvements, like having the refresh button actually show feedback when touched and ideally be grey (for some reason android:tint works but android:app doesn't...), and ideally it would be nice to have the question title text size itself according to content to always fit like the original widget (though that's not critical imo)

I've tried my best to do things sensibly but this is my first time writing Koltin and my first time working on such a big Android app so I totally expect and am prepared to be told parts are wrong and that they should be redone - this is also why I've not written any tests.

I'm happy to keep working on this, but would appreciate help and guidance from some folks with more experience with Android and Koltin 😅 hence opening this as a draft until I've addressed some of the above.

Screenshot_2023-06-25-16-07-05-71_b783bf344239542886fee7b48fa4b892
Screenshot_2023-06-25-16-09-01-06_b783bf344239542886fee7b48fa4b892
Screenshot_2023-06-25-16-09-20-15_b783bf344239542886fee7b48fa4b892
Screenshot_2023-06-25-16-10-45-39_b783bf344239542886fee7b48fa4b892

Checklist

  • I self reviewed the submitted code
  • I ran ./gradlew ktlintCheck detekt before submitting this PR
  • I ran the app on a device/emulator or added unit tests to verify this change

Resolves #142

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 2, 2023

I think I've gotten it as good as I can for now by myself - there is still a bit of an annoying initial loading UX race that goes on that isn't helped by how long it can take to load the list afresh, but I think it sits in the OS space (in particular, I think sometimes the widget is delayed because Android itself has delayed processing the intent) and I've not found anyway to handle that better.

Ultimately this boils down to users just having to sometimes press the refresh button a couple of times initially - this is actually consistent with the original widget, which is also why I think it's more of an Android OS thing.

Also I couldn't figure out the bug where the app would sometimes open on the old question and not switch to the new one - while I've had this happen frequently, I've not found a consistent way to reproduce it and it always can be fixed by just refreshing the widget to a new question so I don't think it should be worried about for now.

Finally, I ended up decompiling the APK for the original app to have a look at the layouts so the widget now resembles the original a lot more (not that it was too differently, but mainly the refresh button is now better and gives visual feedback)

@G-Rath G-Rath marked this pull request as ready for review July 2, 2023 00:40
@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 2, 2023

@tylerbwong I've run the majority of CI locally and confirmed it passes since GHA won't run without approval :)

@tylerbwong
Copy link
Owner

@G-Rath This is awesome, thanks so much for your contribution! I'm unfortunately a little busy right now and haven't had much time to work on Stack lately, but I'll hopefully be able to get to this in the coming weeks. Again, thanks so much for your hard work!

Copy link
Owner

@tylerbwong tylerbwong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Always wanted to find a use-case for a widget and this is perfect. Just a few comments, but it's lookin' good 👍

}

private suspend fun buildRemoteViews(context: Context, question: NetworkHotQuestion): RemoteViews {
return RemoteViews(context.packageName, R.layout.hot_network_questions_widget).apply {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be open to exploring Glance? It uses Jetpack Compose to build out the UI for widgets. Also open to merging this as is and making it a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup happy to do that as a follow-up

@tylerbwong
Copy link
Owner

4x2 Widget Widget Selection
Screenshot_20230709_191502 Screenshot_20230709_191524

This is what it looks like for me when I run it on API 33. Is there a reason why I can't resize them?

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 10, 2023

This is what it looks like for me when I run it on API 33.

That's odd - would you mind playing around with a smaller minWidth setting? My guess is that it's because thats dp based (and probably why in Android 12 you can specify cells rather than dps but the min android version means we can't use that for now).

When I'm back on the laptop I use for doing Android, I'll look into what values the original app was using too.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 13, 2023

@tylerbwong ready for another look - I've also adjusted the minWidth and minHeight of the widget to match the values used by the original widget; this might resolve your size problem, but if not could you play around with those values as I suspect that's what's doing that.

@G-Rath G-Rath requested a review from tylerbwong July 13, 2023 20:15
@tylerbwong
Copy link
Owner

@tylerbwong ready for another look - I've also adjusted the minWidth and minHeight of the widget to match the values used by the original widget; this might resolve your size problem, but if not could you play around with those values as I suspect that's what's doing that.

I think those values looks fine. Can we also look into specifying targetCellWidth and targetCellHeight for newer API levels? More info here.

Seems like there are other sizing attributes to use as well:

android:minResizeWidth="40dp"
android:minResizeHeight="40dp"
android:maxResizeWidth="120dp"
android:maxResizeHeight="120dp"
android:resizeMode="horizontal|vertical"

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 15, 2023

I didn't think that you could use them with the min android level, but maybe I'm wrong.

I'm on mobile right now so if you're comfortable would you mind just pushing up the min cell sizes if that's the last thing needed to land this? Otherwise I can try take a look over the next couple of days (or as a follow up if you're happy to merge as is for now)

I don't think it makes sense to allow resizing though because the widget is only really designed for one size - which makes sense because titles are generally less than three lines...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the asset studio to generate this icon as a vector drawable?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update this real quick after it lands. No worries.

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="#FFFFFF"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be hardcoded? I think we should let the app theme take care of this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update this real quick after it lands. No worries.

@tylerbwong
Copy link
Owner

I didn't think that you could use them with the min android level, but maybe I'm wrong.

I'm on mobile right now so if you're comfortable would you mind just pushing up the min cell sizes if that's the last thing needed to land this? Otherwise I can try take a look over the next couple of days (or as a follow up if you're happy to merge as is for now)

I don't think it makes sense to allow resizing though because the widget is only really designed for one size - which makes sense because titles are generally less than three lines...

I think they would just get ignored on newer API levels, but I agree that resizing probably doesn't make much sense. Never mind then!

Copy link
Owner

@tylerbwong tylerbwong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awesome contribution!

@tylerbwong tylerbwong merged commit 3025407 into tylerbwong:master Jul 15, 2023
7 checks passed
@G-Rath G-Rath deleted the add-hot-network-questions-widget branch July 15, 2023 07:38
@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 15, 2023

No worries, thanks for landing it! How long does it usually take to get published to the play store?

@tylerbwong
Copy link
Owner

No worries, thanks for landing it! How long does it usually take to get published to the play store?

It's been quite awhile since I've published a new release to the Play Store. There was never a set cadence, just when there was something new/stable enough to release.

There have been a few changes since the last release so I have to do some testing, but I will try my best to land it within the next week or so.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 15, 2023

Sweet as - let me know if you find anything that could help address, and I'll see what I can do

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.

Hot network questions view and widget
2 participants