Add sort button and logic #9560

OogaBoogaCode · 2025-02-11T22:03:55Z

This is the reworked PR.

github-actions

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_selectfile.cpp

zenseii

Hi, @OogaBoogaCode. Thank you for this contribution.
I left some comments. The main change regards the button code, which I've outlined. Please have a look when it is convenient for you.

src/fheroes2/gui/ui_window.cpp

src/fheroes2/agg/icn.h

src/fheroes2/dialog/dialog_selectfile.cpp

ihhub

Hi @OogaBoogaCode , I left multiple comments here. Would you mind to take a look at them?

ihhub · 2025-02-12T05:00:28Z

src/fheroes2/system/settings.h

@@ -56,6 +56,12 @@ enum MusicSource
    MUSIC_EXTERNAL
 };

+enum SaveSortMode


This should be class enumeration with no values:

enum class SaveFileSortMethod : uint8_t { SORT_BY_NAME, SORT_BY_DATE };

ihhub · 2025-02-12T05:02:28Z

src/fheroes2/system/settings.h

@@ -373,6 +385,7 @@ class Settings

    int sound_volume;
    int music_volume;
+    int save_sort_mode;


Please use the correct camelCase variable naming convention:

SaveFileSortMethod _saveFileSortMethod;

and since this is going to be uint8_t move it after ZoomLevel _viewWorldZoomLevel{ ZoomLevel::ZoomLevel1 };

Moreover, this member must have a default value:

SaveFileSortMethod _saveFileSortMethod{ SaveFileSortMethod::SORT_BY_NAME };

ihhub · 2025-02-12T05:03:21Z

src/fheroes2/system/settings.h

@@ -94,6 +100,11 @@ class Settings
        return _currentMapInfo;
    }

+    int SaveSortMode() const


Please use the correct method naming:

SaveFileSortMethod getSaveFileSortMethod() const { return _saveFileSortMethod; }

ihhub · 2025-02-12T05:04:18Z

src/fheroes2/system/settings.h

@@ -257,6 +268,7 @@ class Settings

    void SetSoundVolume( int v );
    void SetMusicVolume( int v );
+    void SetSaveSortMode( int mode );


This method should have the following signature:

void setSaveFileSortMethod( const SaveFileSortMethod method ) { _saveFileSortMethod = method; }

The method can be inlined since it is literally one line of code.

ihhub · 2025-02-12T05:07:07Z

src/fheroes2/system/settings.cpp

@@ -390,6 +396,9 @@ std::string Settings::String() const
    os << std::endl << "# music volume: 0 - 10" << std::endl;
    os << "music volume = " << music_volume << std::endl;

+    os << std::endl << "# save sort mode: 1 - by name, 2 - by timestamp" << std::endl;


We must not use any cryptic integer values and offer users more friend string approach:

os << std::endl << "# file save sort method: name or date" << std::endl; if ( _saveFileSortMethod == SaveFileSortMethod::SORT_BY_NAME ) { os << "file save sort method = name" << std::endl; } else { os << "file save sort method = date" << std::endl; }

ihhub · 2025-02-12T05:09:00Z

src/fheroes2/gui/ui_window.h

@@ -89,6 +89,7 @@ namespace fheroes2
                                 const Padding padding );
        void renderButton( Button & button, const int icnId, const uint32_t releasedIndex, const uint32_t pressedIndex, const Point & offset, const Padding padding );
        void renderOkayCancelButtons( Button & buttonOk, Button & buttonCancel, const bool isEvilInterface );
+        void renderOkaySortCancelButtons( Button & buttonOk, Button & buttonSort, Point sortButtonOffset, Button & buttonCancel, const bool isEvilInterface );


Please remove this method. Adding a new method which does exactly the same but for a very specific case is not a good design idea. Instead, you can use renderButton() method call for 3 buttons from your code.

ihhub · 2025-02-12T05:11:52Z

src/fheroes2/dialog/dialog_selectfile.cpp

@@ -291,14 +297,13 @@ namespace

        const bool isEvilInterface = Settings::Get().isEvilInterfaceEnabled();


Please cache Settings::Get() call and use it here and later in the newly added code.

ihhub · 2025-02-12T05:13:52Z

src/fheroes2/dialog/dialog_selectfile.cpp

@@ -247,8 +247,14 @@ namespace
            }
        }

-        std::sort( mapInfos.begin(), mapInfos.end(), Maps::FileInfo::sortByFileName );


getSortedMapsFileInfoList() function is literally used only in one place. We should just pass sorting method as an input parameter for this function.

ihhub · 2025-02-12T05:15:44Z

src/fheroes2/dialog/dialog_selectfile.cpp

+                int const sortMode = Settings::Get().SaveSortMode();
+                Settings::Get().SetSaveSortMode( sortMode == 0 ? 1 : 0 );
+                lists = getSortedMapsFileInfoList();
+                listbox.SetListContent( lists );


Do we need to call this function? What happens with a file selection? Does it preserved while switching between methods?

ihhub · 2025-02-12T05:16:42Z

src/fheroes2/dialog/dialog_selectfile.cpp

+                listbox.updateScrollBarImage();
+                listbox.Redraw();
+            }
+
            if ( le.MouseClickLeft( buttonCancel.area() ) || Game::HotKeyPressEvent( Game::HotKeyEvent::DEFAULT_CANCEL ) ) {


Since we added the condition of mouse click above then we need to add else keyword here as we don't expect to execute the code.

github-actions bot assigned OogaBoogaCode Feb 11, 2025

zenseii added improvement New feature, request or improvement ui UI/GUI related stuff labels Feb 11, 2025

zenseii added this to the 1.1.7 milestone Feb 11, 2025

github-actions bot reviewed Feb 11, 2025

View reviewed changes

src/fheroes2/dialog/dialog_selectfile.cpp Outdated Show resolved Hide resolved

src/fheroes2/dialog/dialog_selectfile.cpp Outdated Show resolved Hide resolved

zenseii self-requested a review February 11, 2025 22:33

zenseii requested changes Feb 11, 2025

View reviewed changes

src/fheroes2/gui/ui_window.cpp Outdated Show resolved Hide resolved

src/fheroes2/agg/icn.h Outdated Show resolved Hide resolved

src/fheroes2/dialog/dialog_selectfile.cpp Outdated Show resolved Hide resolved

src/fheroes2/dialog/dialog_selectfile.cpp Outdated Show resolved Hide resolved

ihhub requested changes Feb 12, 2025

View reviewed changes

ihhub marked this pull request as draft February 23, 2025 04:21

OogaBoogaCode closed this Feb 25, 2025

OogaBoogaCode force-pushed the Add-sort-button-and-logic branch from 3f156f8 to 98b74a7 Compare February 25, 2025 03:43

zenseii mentioned this pull request Apr 4, 2025

Remember the last save file. #9715

Open

1 task

ihhub mentioned this pull request May 9, 2025

Add ability to sort save files by name or date #9793

Merged

		@@ -291,14 +297,13 @@ namespace

		const bool isEvilInterface = Settings::Get().isEvilInterfaceEnabled();

Uh oh!

Add sort button and logic #9560

Add sort button and logic #9560

Uh oh!

Conversation

OogaBoogaCode commented Feb 11, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zenseii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ihhub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!