besteon / Ironmon-Tracker

A Lua script for the Bizhawk/mGBA emulator compatible with Pokemon Fire Red, Leaf Green, Ruby, Sapphire, and Emerald that tracks relevant data for the IronMon challenge.
MIT License
129 stars 45 forks source link

Feature - Log Viewer Search & Filtering #384

Closed Aeiiry closed 1 year ago

Aeiiry commented 1 year ago

LogSearchScreen

Decided to go ahead and finish this feature to a baseline level and make a PR. Adds the ability to search the log viewer by Ability, Pokemon name, or learnable move, in addition to sorting by a given stat, BST, alphabetical, or pokedex number

UTDZac commented 1 year ago

This feature is really freaking awesome. Thank you for putting it together. I'm excited to get to use it and watch people use it to look up useful information.

Change Requests For You

  1. Can you please revert the new pokemon name and icon layout used on the Pokemon tab screen of the log viewer? Before this update, the name appeared above the pokemon icon and it was not surrounded by a box. I'd like to keep the original layout. If there is a need for this change, we can discuss it, just let me know.
  2. For all dropdown boxes, the list of items and their order appear to change. This makes the user experience difficult for quickly eyeballing what is available to select and where its located. Can you rework the dropdown list such that 1) they always contain all items regardless of redundancy with what's currently selected and 2) the order of items never changes.
    • Perhaps the best way to implement this is to separate what sort/filter option is shown as the dropdown box header from the actual list that is populated when clicking the dropdown box. The former here simply displays whatever is currently selected (it does NOT draw a dropdown list item, it draws its own item) and the latter displays the list contents of said dropdown box. As such, you can restructure you button tables to allow for cleaner organizations and drawing logic:
      • Buttons: holds the uppermost dropdown button boxes, and the back arrow button
      • SortByButtons: holds the list items for the dropdown
      • FilterByButtons: holds the list items for the dropdown
      • KeyboardButtons: holds the A-Z keyboard buttons
  3. Some theme colors and shadow colors are incorrect for many areas of the Search screen. Some areas use lower box text instead of Default text, or the wrong background color. Please double-check by using this theme: FFFFFF 222222 00FF00 FF0000 FFFF00 FFFFFF AAAAAA 222222 AAAAAA EEEEEE 000000 1 1
    • image
  4. I would prefer if you reverted all changes to Drawing.lua and instead implemented any needed changes at an individual button level, using the button's .draw function to do any sort of custom drawing. It's possible that in the future we'll want to rework some of the UI elements and button drawing methods, but until then I dont want any global changes to this structure.
    • For example, instead of creating a new button type for PIXELIMAGE_BORDER you can just use ICON_BORDER or PIXELIMAGE then add a custom draw function to create the rectangle around it.
    • The same can be done for the keyboard buttons, which use a textOffsetX that would otherwise not exist. You can remove the text attribute of the button, naming it something else, then draw that text in the button's draw function to meet your specifications.
  5. The blink underline blinks faster if I use speedup. This is fairly common across streamers to use speed up. As such, it blinks pretty fast. There is technically a way to resolve this based on check if speedup is happening, but its a bit tricky. You can find this in the carousel logic: TrackerScreen.getCurrentCarouselItem. Otherwise, perhaps its easiest to just remove the blinking.
  6. The "Search the Log" screen appears and is clickable when viewing non-Pokemon tabs of the log viewer. This can be confusing if someone tries to click on the search screen to do things, and nothing happens. I dont have a good answer for this yet, but let's please discuss this. Am interested if you have an idea.
  7. I'd like the Utils.findInTable function to be removed. As a black-box approach, I don't understand how to use this function, as it does things that aren't described in the name of the function. It's hard for me to understand how I'd want to use it. With that aside, I also do not understand how this is helping in your current code, or what it's doing exactly. We can discuss in discord if you'd like. I don't think this is necessary, as I removed it and its related code and everything worked fine anyway.

In general, for new feature PRs, large scale changes such as changes to Drawing or Utils are likely not needed. If you do believe changes like this must be made to accommodate the feature, then please reach out beforehand and discuss with us. In many cases, this larger global changes may affect other features being worked on internally or by others. When able, try and find a solution that doesn't require these changes. But I do understand there will be cases with it makes more sense to make larger scale changes like this for your feature (and potential other new features).

Changes I Made

  1. The sort-by dropdown onclick didn't have a redraw event under some conditions, thus there was an awkward delay when clicking and when the box popped up. I fixed this.
  2. Some indent/spacing issues with some files: Constants.lua
  3. Removed some excess comments which don't appear to be needed anymore
  4. Removed local from variables that were also parameters in buildKeyboardButtons
  5. For the on-screen keyboard, I centered the characters within the keyboard boxes, I think this looks nicer
  6. I renamed "Pokemon Name" to "Pokémon Name"
  7. I renamed "Learnable move" filter option to "Levelup Move" to remove ambiguity that it doesn't actually filter by learnable tm moves. This matches what's shown on the log viewer single pokemon screen.

If any of the above changes I made don't seem right to you, let me know and we can discuss or revert. These I felt were obvious enough to just go ahead and do.

Optional Additions to Your Feature

  1. Add a [X] button to the right of the back-arrow button. This is a clear button. Having a single button to clear out the search query would be ideal. Often people do not have double-click to fullscreen disabled, and as such clicking the back-arrow many times will be annoying and difficult.
  2. When changing the filter by dropdown box, it should probably also clear out the search query. Since searching for "shuck" no longer makes contextual sense when i swap from "Pokemon Name" filter to "Ability" filter.
Aeiiry commented 1 year ago

All of your requests are very reasonable, I'll go ahead and implement those :). Some of the changes to other files were me getting a bit too ambitious with the scale of things but I agree that those changes can be localised to individual buttons' .draw functions or the screen's draw logic.

Aeiiry commented 1 year ago

I've made changes that I believe adhere to all of your above change requests. I opted to remove the blinking for now as in the small time I spent trying to get it to work it was a bit of a headache dealing with the frame timers. When viewing a tab outside of the "Pokemon" tab the "Search the Log" screen is no longer shown, instead the default/fallback pokemon info screen is shown (Player's starter or bulbasaur)

UTDZac commented 1 year ago

Thank you for making all of those changes. I tested them out and found a few more minor things I wanted to clean up a little. I'll list them out and commit them following this post. Let me know what you think and if you don't think these adjustments are what you want.

image

  1. When a sort had equal values, such as two Pokémon with identical BST, they could get swapped around randomly as you clicked or typed. I added a default secondary sort, by pokedex number, to ensure the sorted outcome is always exact
  2. I increased the height of the dropdown boxes so that the text fit better for words like "Pokémon"
  3. I added back in the currently selected dropdown item into each dropdown box. The dropdown item list should never change; it should be static and in the same order, so its easy to click into it and know where things are
  4. Per the above change, I changed the color of the dropdown item list so it stands out from the currently selected dropdown item
  5. When clicking the dropdown box, there was still a delay before it popped up. This is because most/all onclick events also need to include a Program.redraw(true) to update the screen to show the click has happened.
  6. In the Search Text area, I centered each of the characters that have been typed in thus far. Same trick used for keyboard character centering
  7. (line 606) Added a shadow box to dropdown boxes so they have that "pop" look-and-feel like most buttons
  8. Removed redundant Input button checks. The Input check already skips all buttons that aren't visible or being drawn, and dropdown items aren't visible if the dropdown boxes isn't open. (technically this is not the most efficient approach but it does allow for easier code readability imo)