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

Improved optionsmenu #240

Merged
merged 22 commits into from
Jan 30, 2017
Merged

Conversation

yugecin
Copy link
Contributor

@yugecin yugecin commented Jan 29, 2017

Some enchancements to more look like the optionsmenu in osu!

  • Added subgroups (I also tried to move some options in a way that looks logic to make use of subgroups)
  • Fading when showing/hiding, also using clip instead of moving the entire overlay
  • Animated comboboxes
  • Improved slider rendering + removed the value next to the slider and added them inside the tooltip
  • Added animated selected option indicator
  • Optionsmenu is now searchable
  • Also added fix for Skins&drop-down menu #237 (dropdown list exceeding maximum scrolling height)
  • Added overscrolling

@yugecin yugecin changed the title Enchanced optionsmenu Improved optionsmenu Jan 29, 2017
@@ -705,6 +705,7 @@ public void mouseWheelMoved(int newValue) {

@Override
public void keyPressed(int key, char c) {
UI.getNotificationManager().sendNotification("ayyyy lmao", Colors.GREEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, thought I removed it >.< was added it to test the bubble notifs, didn't mean to commit it.

@itdelatrisu
Copy link
Owner

itdelatrisu commented Jan 29, 2017

Awesome, thanks! Visually, this looks fantastic. :)

Comments (sorry, there's a lot XD):

  • You forgot to commit search.png.
  • I preferred when the value of numeric options was on the screen, not just in a tooltip -- you can't really tell what a lot of them are without hovering since the min/max aren't obvious.
  • You need to pass true as the last parameter of UI.updateTooltip() to parse newlines properly (OptionsOverlay.java:725).
  • This introduces a weird bug where scores in the song menu are flickering and changing positions... which is fixed if you remove the else in KineticScrolling.java:130: } else if (position < min) {
  • You can't grab focus in the OptionsOverlay constructor, or else the search field will consume key events everywhere (e.g. the main menu). Remove this line (OptionsOverlay.java:314): searchField.setFocus(true);
  • I don't understand the purpose of the TextField.java changes. Commenting out searchField.performKeyRepeat(); in OptionsOverlay.java:705 doesn't change anything for me (holding down keys still works).
  • You might want to cap the length of the search field to about the width of the overlay. Alternatively, osu! has a hard cap of 20 characters. (osu! also doesn't even let you enter characters that would produce zero results, though I don't really care about this feature.)
  • It feels kind of redundant to not use the DropdownMenu class for the list options since they should look identical, but it might be more effort than it's worth to put them in. (Maybe they could share a static method for rendering...)

@yugecin
Copy link
Contributor Author

yugecin commented Jan 29, 2017

Replies (also alot :p)

  • Added search.png, whoops
  • I had mixed feelings about the on-screen values and I thought it wasn't that bad when they were gone, but I added them again now (and removed from the tooltip).
  • Changed the newline argument for the tooltip.
  • Reverted the else if in KineticScrolling.java:130
  • I've removed the focus grabbing from the constructor and added them in the activate() and deactivate() methods. Note that I make calls to setFocus in the keyPressed method, right before I send the keypresses to the text field. If I remove that line it does not accept the keys and I can't seem to find why.
  • I reverted the TextField changes, I forgot that the line container.getInput().enableKeyRepeat(); in Utils.java does the key repeat so it didn't need to be handled in the code like I did.
  • Changed the search field max length to 20. I didn't pay much attention to it as it doesn't really break anything. I also ignored the feature in osu! that doesn't let you enter new characters when there are 0 results because it would need more checking and is somewhat unecessary.
  • I didn't really want to use the DropdownMenu class because it would probably be a hassle to keep track of the instances and changing their locations. I'm trying to make use of DropdownMenu atm.

And I might've removed some comments in certain methods because I rewrote some large parts and just pasted it over the existing code, so I apologize if that happend.

@yugecin
Copy link
Contributor Author

yugecin commented Jan 29, 2017

I've changed it so it uses the existing DropdownMenu now, I should've done that from the beginning.
It also fixes #237 yet again because I suspect that my previous fix had an issue too.

Edit: still have to fix something, it shows the first value in the list when just created fixed

@itdelatrisu
Copy link
Owner

itdelatrisu commented Jan 30, 2017

EDIT: figured it out, never mind.

Great, thanks for fixing all of that so quickly. Last question: I'm can't understand why this line works or where the numbers are coming from, can you explain? (OptionsOverlay.java:556)

maxScrollOffset -= height * 2 / 3;

Purpose: I don't like the huge empty space at the bottom of the menu (is it intentional?), which is fixed by starting with maxScrollOffset = 0, but then I'm still confused about the line above.

@itdelatrisu itdelatrisu merged commit bc8c5cd into itdelatrisu:master Jan 30, 2017
itdelatrisu added a commit that referenced this pull request Jan 30, 2017
- Mostly style/naming changes and some refactoring.
- Reinitialize the dropdown menu selected item on activate() since some options can change outside of the menu.
- Fixed non-relative coordinates being used.

Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
@yugecin yugecin deleted the enchanced-optionsmenu branch January 30, 2017 08:16
@yugecin
Copy link
Contributor Author

yugecin commented Jan 30, 2017

That was because osu! does it too, but 1/4th of the height might've been more representative than 2/3 http://pokit.org/get/img/c7d9efc0797dbdc11250f22b30cecedf.jpg

@itdelatrisu
Copy link
Owner

Ah, didn't notice that before. Well, got rid of it anyway since I don't like it. xD

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

Successfully merging this pull request may close these issues.

3 participants