Added category filtering for hotkeys in settings window #9957
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi @pazam-h2 , this implementation contradicts the overall UX design of the game: none of text elements are clickable. An ideal solution it to embed these categories into the list itself. |
|
Changed from text elements to just a list. |
zenseii
added
improvement
…able, not just the text
|
Hi @pazam-h2 , could you please provide a screenshot of the window for the original 640x480 resolution? |
| enum class HotKeyCategory : uint8_t | ||
| enum class HotKeyCategory : int | ||
| { | ||
| ALL = -1, |
There was a problem hiding this comment.
Please revert this change. This seem like a hack rather than a solution. Usually, all items in enumeration start from 0 (default value). We call simply call Game::getAllHotKeyEvents(); for that particular entry in the dialog.
| const char * Game::getHotKeyCategoryName( const HotKeyCategory category ) | ||
| { | ||
| switch ( category ) { | ||
| case HotKeyCategory::ALL: |
There was a problem hiding this comment.
Please revert this change. This doesn't belong to the class but only for a single specific dialog. So, let's make changes where they needed - in that dialog.
| std::set<Game::HotKeyCategory> seen; | ||
| for ( const auto & pair : hotKeyEvents ) { | ||
| if ( seen.find( pair.second ) == seen.end() ) { | ||
| seen.insert( pair.second ); | ||
| uniqueCategories.push_back( pair.second ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This code isn't needed. We can directly take all entries from the enumeration.
| std::vector<std::pair<Game::HotKeyEvent, Game::HotKeyCategory>> hotKeyEvents = Game::getAllHotKeyEvents(); | ||
|
|
There was a problem hiding this comment.
With the above comment we can revert this change since hotKeyEvents is not being used above.
| const fheroes2::Text categoryHeader( _( "Category:" ), fheroes2::FontType::normalWhite() ); | ||
| categoryHeader.draw( roi.x + 45, roi.y + 32, display ); | ||
|
|
||
| for ( const Game::HotKeyCategory category : uniqueCategories ) { |
There was a problem hiding this comment.
We don't need uniqueCategories but directly use enumeration entries. Just add one more entry before the loop for _( All ).
| const bool isSelected = ( category == selectedCategory ); | ||
| const fheroes2::FontType fontType = isSelected ? fheroes2::FontType::normalYellow() : fheroes2::FontType::normalWhite(); | ||
| const fheroes2::Text categoryText( _( Game::getHotKeyCategoryName( category ) ), fontType ); | ||
| categoryText.draw( categoryRoi.x + 4, categoryOffsetY, display ); | ||
|
|
||
| const fheroes2::Rect textRect( categoryRoi.x, categoryOffsetY, categoryRoi.width, categoryText.height() + 4 ); | ||
| categoryRects.emplace_back( category, textRect ); | ||
|
|
||
| categoryOffsetY += categoryText.height() + 4; | ||
| } |
There was a problem hiding this comment.
Please move this code into a separate function in anonymous namespace and call it here and in the code below to avoid code duplication.
|
|
||
| display.render( background.totalArea() ); | ||
|
|
||
| std::vector<std::pair<Game::HotKeyEvent, Game::HotKeyCategory>> filtered = hotKeyEvents; |
There was a problem hiding this comment.
This variable is not needed especially outside the loop. Its lifetime is within the newly created below if-condition body.
| listbox.QueueEventProcessing(); | ||
|
|
||
| for ( const auto & pair : categoryRects ) { | ||
| if ( le.MouseClickLeft( pair.second ) ) { |
There was a problem hiding this comment.
We should update the list only when clicking is done on a different item. Now we rebuild everything without this conditon.
| for ( auto & categoryPair : categoryRects ) { | ||
| const bool isSelected = ( categoryPair.first == selectedCategory ); | ||
| const fheroes2::FontType fontType = isSelected ? fheroes2::FontType::normalYellow() : fheroes2::FontType::normalWhite(); | ||
| const fheroes2::Text categoryText( _( Game::getHotKeyCategoryName( categoryPair.first ) ), fontType ); | ||
| categoryText.draw( categoryRoi.x + 4, categoryOffsetY, display ); | ||
|
|
||
| categoryPair.second = { categoryRoi.x, categoryOffsetY, categoryRoi.width, categoryText.height() + 4 }; | ||
| categoryOffsetY += categoryText.height() + 4; | ||
| } |
There was a problem hiding this comment.
I would like to see how this dialog looks in the original resolution: 640x480 and whether every element fits. Also, please attach screenshots in other languages to see how it looks and whether the dialog is not being crumped for some languages.
| #include <cassert> | ||
| #include <cstdint> | ||
| #include <memory> | ||
| #include <set> |
There was a problem hiding this comment.
This header won't be needed with the changes proposed below.
|
I don't think that this is a good design solution for the original resolution: 
|
3 participants
This PR adds a visual category selector to the Hotkeys settings dialog, allowing users to filter hotkeys by category.
The category names are displayed in a 2-line layout above the hotkey list. Selecting a category filters the list accordingly.
EDIT 1: the placement of the categories have been changed to be on the left side as an organized list.
EDIT 2: added an 'All' button to have the hotkeys list unfiltered.
EDIT 3: added Category sub-title
from:
To:
Closes #9940