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

Mobile queue fixes #4742

Merged
merged 46 commits into from Jul 7, 2023
Merged

Mobile queue fixes #4742

merged 46 commits into from Jul 7, 2023

Conversation

aliabid94
Copy link
Collaborator

@aliabid94 aliabid94 commented Jun 30, 2023

Closes: #2075

aliabid94 and others added 30 commits June 14, 2023 15:18
* create abstract toast component for info/warning/error

* add icons

* add light mode theming

* add theme mode check and dark mode styles

* update theme_mode on update

* clean up render of toast content

* replace dynamic css vars with toast type css selectors

* tweak colours

* change error pill border colour to align with toasts

* formatting

* fix failed test

* rename icon files and clean up css
@abidlabs
Copy link
Member

abidlabs commented Jul 6, 2023

Working on testing this

const SHOW_DUPLICATE_MESSAGE_ON_ETA = 15;
const SHOW_MOBILE_QUEUE_WARNING_ON_ETA = 10;
Copy link
Member

@abidlabs abidlabs Jul 6, 2023

Choose a reason for hiding this comment

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

Perhaps also set this to 15 seconds like SHOW_DUPLICATE_MESSAGE_ON_ETA so that we have a consistent definition of a "long waiting time" that we can communicate to users

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I chose 10 seconds specifically because that's how long it seems iPhones break a websocket connection on low power mode. I don't think there's any need for these to match

Copy link
Contributor

Choose a reason for hiding this comment

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

That should probably be documented in a comment near the constant :)

@abidlabs
Copy link
Member

abidlabs commented Jul 6, 2023

Very nice @aliabid94! Can confirm that I see this warning modal when I initiate the job:

image

(though only with queuing enabled, which I think is fine)

And I see this when I switch tabs. (Btw on my iPhone, I only see this if I set it to low power mode, otherwise the connection doesn't break):

image

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

LGTM!

@freddyaboulton
Copy link
Collaborator

Maybe this is part of the scroll_to_output problem on spaces but on the deployed demo the warning gets automatically closed

IMG_6055.MOV

@abidlabs
Copy link
Member

abidlabs commented Jul 6, 2023

Hmm @freddyaboulton it's working fine for me on iPhone (tested on Chrome and Safari)

image

Are you on iPhone?

@freddyaboulton
Copy link
Collaborator

yes - iphone and on safari 🤔 I have an ancient iphone 10 though. let me try again.

Copy link
Collaborator

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

Modal stopped disappearing for me @aliabid94 ! Thanks for the fix

CHANGELOG.md Outdated
@@ -52,6 +52,7 @@
- Better errors when you define two Blocks and reference components in one Blocks from the events in the other Blocks [@abidlabs](https://github.com/abidlabs) in [PR 4738](https://github.com/gradio-app/gradio/pull/4738).
- Better message when share link is not created by [@abidlabs](https://github.com/abidlabs) in [PR 4773](https://github.com/gradio-app/gradio/pull/4773).
- Improve accessibility around selected images in gr.Gallery component by [@hannahblair](https://github.com/hannahblair) in [PR 4790](https://github.com/gradio-app/gradio/pull/4790)
- Warning on mobile that if a user leaves the tab, websocket connection may break. On broken connection, tries to rejoin queue and displays error conveying connection broke. By [@aliabid94](https://github.com/aliabid94) in [PR 4742](https://github.com/gradio-app/gradio/pull/4742)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this to the correct changelog release section!

const SHOW_DUPLICATE_MESSAGE_ON_ETA = 15;
const SHOW_MOBILE_QUEUE_WARNING_ON_ETA = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense

@aliabid94 aliabid94 merged commit 280b569 into main Jul 7, 2023
10 checks passed
@aliabid94 aliabid94 deleted the mobile_queue_fixes branch July 7, 2023 19:39
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.

Multi-tasking on mobile (iOS/Android) seems to silently remove user from the queue
6 participants