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

Android: Add Install WAD to menu_game_grid #8748

Merged
merged 2 commits into from Apr 27, 2020
Merged

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented Apr 17, 2020

Implements https://bugs.dolphin-emu.org/issues/10788
Useful for setting up mod loaders.

@JosJuice
Copy link
Member

Haven't tested it, but the code seems fine. One comment, though:

I know that one of the Android contributors we've had before has been pickier than me about making sure that long-running operations run on a separate thread so that the UI remains responsive. Importing a WAD likely counts as such an action, since it involves reading and writing tens of megabytes to storage. You might want to consider moving it to a different thread, but I personally don't consider this to be a blocker.

@Ebola16
Copy link
Member Author

Ebola16 commented Apr 21, 2020

I've never written multi-threaded code so please correct me if I'm misunderstanding something here. Wouldn't we want WAD installation to block on the UI thread here? We don't want users to begin emulation if a WAD is being installed. A toast is also given for success or failure so users will be notified when WAD installation finishes.

@JosJuice
Copy link
Member

We don't want users to begin emulation if a WAD is being installed.

I guess that is a good idea, but that can be done without blocking the UI thread. (For instance, it's possible to show an "installing" dialog that prevents the user from interacting with the UI behind... Though I guess that might be unnecessary if WAD installation is quick enough.) The reason why blocking the UI thread is bad is because it can make users think that the app has frozen, and Android can even show the "application not responding" message if it goes on for a few seconds.

But again, I don't consider this a blocker, since WAD installs don't take all that long.

@Ebola16
Copy link
Member Author

Ebola16 commented Apr 22, 2020

@JosJuice let's avoid the (very unlikely) chance for ANRs. Please check to make sure my latest commit is the best way to accomplish this.

@Ebola16
Copy link
Member Author

Ebola16 commented Apr 22, 2020

@JosJuice Ok, this implementation should be a little more standard.

@Ebola16
Copy link
Member Author

Ebola16 commented Apr 22, 2020

The dialog.dismiss() issue has been resolved.

@Ebola16
Copy link
Member Author

Ebola16 commented Apr 23, 2020

@JosJuice Done

@Ebola16
Copy link
Member Author

Ebola16 commented Apr 26, 2020

I think this is good to merge.

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

It's been a very long time since I did Android development, but the changes look good to me.

@leoetlino leoetlino merged commit b175e9e into dolphin-emu:master Apr 27, 2020
@Ebola16 Ebola16 deleted the WAD branch April 27, 2020 15:54
@Sanmorin
Copy link

Good afternoon. This option is not available in the Android TV Dolphin menu. Can you add it? Tested on Dolphin 5.0-12031

@Ebola16
Copy link
Member Author

Ebola16 commented May 19, 2020

I'll look into that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants