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: Skylanders Portal UI #11490

Merged
merged 1 commit into from Feb 2, 2023

Conversation

deReeperJosh
Copy link
Contributor

@deReeperJosh deReeperJosh commented Jan 25, 2023

Following on from 11331, I would like to make the Emulated portal on Android. Creating this PR as draft to gain some feedback/ideas on remaining pieces, will attach some Screenshots when I get the chance.

  • Portal recognised in Game
  • Jni methods to handle adding/removing Skylanders
  • Create a new Dialog Menu for Skylanders (exclude it from settings)

@deReeperJosh deReeperJosh force-pushed the skylandersportalandroid branch 4 times, most recently from 5a83dbb to 83512af Compare January 25, 2023 02:53
@t895
Copy link
Contributor

t895 commented Jan 25, 2023

I'd really prefer you make a series of dialogs or a fragment that creates a skylander instead of an activity. I'm currently working on moving to the single-activity model for the app and this would make things worse.

@deReeperJosh
Copy link
Contributor Author

@t895 apologies, just a typo there. I'm creating a fragment and not an activity 😁

@deReeperJosh
Copy link
Contributor Author

@t895 just checking, now that I'm trying to get it out of settings, in my mind it would be better to launch the whole Skylanders tool as a new Activity from the in game emulation menu, like Settings does at the moment. What would be your suggestion as to what I should be doing?

@t895
Copy link
Contributor

t895 commented Jan 25, 2023

My first thought would be to still have a toggle for the Skylanders portal in the settings activity and then that reveals a Skylanders list option for the in-game menu fragment. Selecting that option can open an alert dialog that shows a list of all the available Skylanders with buttons to save/load/delete.

Also I generally prefer not to use more activities because of extra duplicated code we have to write based on directory initialization, theme checking and again the single activity model I'm working towards.

@deReeperJosh
Copy link
Contributor Author

@t895 thanks for that - will see how I get on. For now, have reset the branch so that it's just the Settings menu item, and will build it out using a Dialog from here.

@deReeperJosh
Copy link
Contributor Author

Updates for now, have made a new dialog, not sure how good it will look with more than one Skylander, but looks like this:
Screenshot_20230126-153735

Screenshot_20230126-153747

Screenshot_20230126-153800

The AutoCompleteTextView dropdown is being covered by the Keyboard, so that definitely needs to change, but otherwise it's a start

@t895
Copy link
Contributor

t895 commented Jan 26, 2023

What are you trying to do in this dialog?

@deReeperJosh
Copy link
Contributor Author

It would ideally be the java version of the Qt Skylanders Manager - so you can create, load and remove Skylanders. Haven't added the create button in but my theory would be just to use the Load button to create a Skylander if it doesn't exist.

@t895
Copy link
Contributor

t895 commented Jan 26, 2023

No, I mean what is that specific dialog is supposed to do? Do you type out a skylander you want and it suggests saved skylanders?

Also it might be best to do something like have a recycler view of items with buttons instead of entering text. Then the buttons can summon other dialogs to do each action.

@deReeperJosh
Copy link
Contributor Author

Oh sorry I misunderstod - this dialog is meant for selecting a Skylander's name from a list of known Skylanders, which would then in theory create/load the 1kb representation of that Skylander to use in game

@t895
Copy link
Contributor

t895 commented Jan 26, 2023

Ok, I wasn't entirely sure if it was for naming a skylander. But given that the autocomplete dialog will be blocked by most phones, I would try to find another solution that doesn't involve the keyboard at all (except for initially naming the skylanders of course).

@deReeperJosh
Copy link
Contributor Author

I was thinking about moving the dropdown to be a drop up (don't know if that's a real term) but the other option is to let the users enter an ID and Variant integer, and then use that to make a Skylander with

@t895
Copy link
Contributor

t895 commented Jan 26, 2023

I'm not really a fan of that UX because there can be a situation where the display isn't tall enough to force the keyboard into fullscreen mode. Then after entering the text, you have to back out and then access the dropdown that already has a reduced scroll space (less than half of the display).

Maybe you could have some kind of select skylander button and summon a dialog that returns a selection. Like, just a list that lets you select a single item with radio buttons.
Radio button example -
signal-2023-01-25-221637_002

@deReeperJosh
Copy link
Contributor Author

That seems heaps better, are radio buttons able to be dynamically updated? Say if I had 3 radio buttons, if the first is "Game" which would have 5 Options, the next being "Element" which has 8 options, then "Skylander" as the next radio button, which if filtered by Game and Element would only have 4 ish options. I think there's like 500+ total Skylanders, so definitely need to try to limit the amount of options on screen

@t895
Copy link
Contributor

t895 commented Jan 26, 2023

I think it can only be a length that is determined before revealing the dialog. But don't you only have to worry about scrolling through 175 main skylanders and then you enter their variant number?

@deReeperJosh
Copy link
Contributor Author

Yeah that's true, every Skylander has a unique id and x amount of variants, are you saying to have a list of all the Skylanders then select a Variant via radio buttons or something like that?

@t895
Copy link
Contributor

t895 commented Jan 26, 2023

I'm honestly pretty conflicted because that is a ridiculous amount of items to scroll through. I think I'd be willing to settle for now where there is a list of the skylander names in a list of radio buttons, and then some kind of edittext box for both of the ids.

Actually, now you have me thinking, how many possible IDs and Variants are there? If it's reasonable (under 100) you could put it in a list of radio buttons like you mentioned or in a slider?

@deReeperJosh
Copy link
Contributor Author

Another iteration on the UI @t895. This is the Spinner idea I briefly mentioned, still need to include the ID/Variants in Spinners below the 2 you can see here.

Screenshot_20230126-232845
Screenshot_20230126-232943

@deReeperJosh
Copy link
Contributor Author

The slot window is scrollable, and there are 2 more buttons below those 6 in the screenshot, I plan to make the Spinner layout scrollable as well.

@deReeperJosh
Copy link
Contributor Author

Screenshot_20230127-101454

Added the name spinner as above. Personally I think it looks great, but definitely going to need a lot of string-array resources to make it work.

@t895
Copy link
Contributor

t895 commented Jan 27, 2023

Make sure to follow these guidelines for padding your dialogs https://m3.material.io/components/dialogs/specs

Do not use the DolphinButton style on that type of button. Just leave it at the default for better theme handling.

I think for the sake of keeping the scope reasonable, you should only implement what you have on the desktop. So only a spinner for the names of the Skylanders and then two others just for the available id/variant numbers.

I'm also still unsure of this grid of buttons you have for each slot. Wouldn't this make more sense to have the list idea I mentioned earlier? Something like this -
Screenshot_20230127-013509.png

@deReeperJosh
Copy link
Contributor Author

@t895 that's a way better idea - is that an existing UI within Dolphin?

@t895
Copy link
Contributor

t895 commented Jan 27, 2023

Not yet, that's some UI I worked on for the Android Input Overhaul. If you'd like you could check that PR and use it as a reference.

@deReeperJosh
Copy link
Contributor Author

deReeperJosh commented Jan 28, 2023

Besides having no clue why the text in the Create Dialog is invisible, I think this one is basically ready for review now. The Window rescales the box sizes when the Id/Variant number changes, but the text color just remains invisible for me.

Ideally it would look like it does in the designer:
Screenshot 2023-01-28 at 3 53 35 PM

But it looks like it does in the screenshots above ^

@deReeperJosh deReeperJosh marked this pull request as ready for review January 29, 2023 00:30
@deReeperJosh
Copy link
Contributor Author

@t895 ready for review/rebuild again

Copy link
Contributor

@t895 t895 left a comment

Choose a reason for hiding this comment

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

You have a couple weird bugs

  • The app will crash if you try to create a skylander with an invalid name or missing fields
  • You can populate every slot only if you create new skylanders repeatedly, but if you load skylanders, they will always replace the skylander in the first slot.

I tried to get a quick review in so I probably missed some things. I'll take a look after the comments have been addressed.

@deReeperJosh
Copy link
Contributor Author

Haven't addressed the bugs but have made the changes as requested. Android Studio made a strange hash of the reformatting so let me try to sort that out.

@deReeperJosh
Copy link
Contributor Author

Bug fixes made as above - create dialog will just disappear if user hasn't entered proper text. Let me know what you think the ideal solution here is :)

Copy link
Contributor

@t895 t895 left a comment

Choose a reason for hiding this comment

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

Your bug fixes work great and I think I'm good with everything after these are addressed. I don't know much with our JNI code so I'll have to ask if @JosJuice thinks it's ok.

@deReeperJosh
Copy link
Contributor Author

@t895 changes made as suggested - have squashed the commits in to one as well. Hopefully JNI stuff is all good as well :)

@deReeperJosh deReeperJosh changed the title Skylanders Portal on Android Android: Skylanders Portal UI Feb 1, 2023
public static final int MENU_ACTION_SKYLANDERS = 36;

private static Pair<Pair<Integer, Integer>, String> skylanderData;
private static int skylanderSlot = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please prefix the names of these two variables with s if they're static... Which they shouldn't be, because you have a more important problem:

It doesn't seem like you're saving these in onSaveInstanceState and restoring them later. Some devices with low RAM might kill the Dolphin process while the file picker is being shown, so this is important. Note that the developer setting "Don't keep activities" is not sufficient for testing this, because it only destroys the activity, not the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think this is sorted now. Wasn't sure if I also needed to include the array as well but that hasn't been included as of yet.

Copy link
Member

Choose a reason for hiding this comment

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

You mean mSkylanderSlots, right? I'm actually not entirely sure how that one is intended to work... Is it supposed to reflect the state of the Skylanders that currently have been "set" for the emulated portal? If so, how do you handle the case where the emulation activity gets created when the emulation core already has Skylanders set? I can't find any code that's fetching that information from the emulation core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah mSkylanderSlots. The theory is that it is a representation of the Skylanders on the portal, and the array list is used to display the name of the Skylander and what slot the user has placed them in the UI. My theory was if the array list was static, the names and positions would persist when the emulation activity is created with Skylanders already placed on the portal, but it sounds like I will need to fetch the information from the core on load

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Qt, the Window has an array m_sky_slots that has the portal slot, id, var of all of the Skylanders present on the portal, which is what I tried to do with mSkylanderSlots

Copy link
Member

@JosJuice JosJuice Feb 1, 2023

Choose a reason for hiding this comment

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

If mSkylanderSlots was static, I believe it would work as long as you don't use any savestates (or is savestating the currently selected Skylanders not implemented?), but currently it isn't static. We do want savestates to work properly though, especially since savestates are used to implement not losing your game process when Android kills the Dolphin process in the background.

Copy link
Member

@JosJuice JosJuice Feb 1, 2023

Choose a reason for hiding this comment

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

Yeah, it seems like DolphinQt has the same problem. That's not great. But since fixing it is a bit of a bigger task and DolphinQt is broken too, I wonder if it would make sense to improve this in another PR instead...? @t895, what do you think?

Source/Android/jni/SkylanderConfig.cpp Outdated Show resolved Hide resolved
Source/Android/jni/SkylanderConfig.cpp Outdated Show resolved Hide resolved
Source/Android/jni/SkylanderConfig.cpp Outdated Show resolved Hide resolved
Source/Android/jni/SkylanderConfig.cpp Show resolved Hide resolved
Source/Android/jni/SkylanderConfig.cpp Outdated Show resolved Hide resolved
@deReeperJosh
Copy link
Contributor Author

@JosJuice changes made as above - let me know if there is anything still missing or not quite right from what I changed :)

@deReeperJosh deReeperJosh force-pushed the skylandersportalandroid branch 2 times, most recently from 3554a18 to 94736e1 Compare February 1, 2023 22:22
Copy link
Contributor

@t895 t895 left a comment

Choose a reason for hiding this comment

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

Just one last thing I noticed before merging.

Source/Android/app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
Co-Authored-By: Charles Lombardo <clombardo169@gmail.com>
@deReeperJosh
Copy link
Contributor Author

@t895 sorry, just rebased with master as well to get the latest clang-format changes in too

@t895 t895 merged commit 6437261 into dolphin-emu:master Feb 2, 2023
14 checks passed
@deReeperJosh deReeperJosh deleted the skylandersportalandroid branch February 6, 2023 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants