| [PATCH] Optimize SDL GUI file selector SDLGui_DoDialog() usage |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/hatari-devel Archives
]
- Subject: [PATCH] Optimize SDL GUI file selector SDLGui_DoDialog() usage
- From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
- Date: Sat, 4 Sep 2021 19:58:37 +0300
- Fileselector is the only thing using "unknown" (unhandled) events +
keeping current object state (for scrollbar handling), so I added
simple wrapper for other dialogs to use, and renamed the full
function to SDLGui_DoDialogExt().
- Added callback arg for it checking whether caller is interested
about given "unknown" (unhandled) event type, so that dialog does
not need to be removed & re-construsted when SDL provides an event
that nobody cares about. [1]
Checking the callback was simpler to do just in one place, which
allowed removing pEventOut checks from elsewhere.
- With file selector providing the returned value back to
SDLGui_DoDialogExt() on next call, "current_object" could be changed
from global variable to a function argument (replacing
"KeepCurrentObject" one).
Doing that required fixing "retbutton" value in SDLGui_DoDialogExt()
scrollbar event handling.
- Moved also few lines doing similar "retbutton" variable handling
closer together for clarity
[1] Filtering out useless events fixes also SDL file selector issue
reported by Miro Kropáček. If unhandled event comes between mouse
button being pressed on a button, but before button is released, that
file selector button remains in a pressed state until user clicks on
it again (even if dialog is closed and re-opened).
In Miro's case this was caused by spurious (keymap) configuration
events which user was not aware of, so the behaviour was especially
surprising.
After this commit, that behaviour can still be triggered by pressing a
key during mouse click (as file selector event type callback accepts
unhandled keys). Because that is an user-inflicted cosmetic issue,
it's acceptable, unlike completely unknown SDL events.
---
src/gui-sdl/dlgAbout.c | 2 +-
src/gui-sdl/dlgAlert.c | 2 +-
src/gui-sdl/dlgCpu.c | 2 +-
src/gui-sdl/dlgDevice.c | 2 +-
src/gui-sdl/dlgFileSelect.c | 22 +++++++++----
src/gui-sdl/dlgFloppy.c | 4 +--
src/gui-sdl/dlgHalt.c | 2 +-
src/gui-sdl/dlgHardDisk.c | 2 +-
src/gui-sdl/dlgJoystick.c | 2 +-
src/gui-sdl/dlgKeyboard.c | 2 +-
src/gui-sdl/dlgMain.c | 2 +-
src/gui-sdl/dlgMemory.c | 2 +-
src/gui-sdl/dlgNewDisk.c | 2 +-
src/gui-sdl/dlgRom.c | 2 +-
src/gui-sdl/dlgScreen.c | 4 +--
src/gui-sdl/dlgSound.c | 2 +-
src/gui-sdl/dlgSystem.c | 2 +-
src/gui-sdl/sdlgui.c | 66 ++++++++++++++++++++++---------------
src/includes/sdlgui.h | 3 +-
19 files changed, 74 insertions(+), 53 deletions(-)
diff --git a/src/gui-sdl/dlgAbout.c b/src/gui-sdl/dlgAbout.c
index cbe6943e..2871af33 100644
--- a/src/gui-sdl/dlgAbout.c
+++ b/src/gui-sdl/dlgAbout.c
@@ -57,5 +57,5 @@ void Dialog_AboutDlg(void)
aboutdlg[1].x = (aboutdlg[0].w - strlen(aboutstr)) / 2;
SDLGui_CenterDlg(aboutdlg);
- SDLGui_DoDialog(aboutdlg, NULL,false);
+ SDLGui_DoDialog(aboutdlg);
}
diff --git a/src/gui-sdl/dlgAlert.c b/src/gui-sdl/dlgAlert.c
index 70d79b11..f5c7fa33 100644
--- a/src/gui-sdl/dlgAlert.c
+++ b/src/gui-sdl/dlgAlert.c
@@ -167,7 +167,7 @@ static int DlgAlert_ShowDlg(const char *text)
bOldMouseVisibility = SDL_ShowCursor(SDL_QUERY);
SDL_ShowCursor(SDL_ENABLE);
- i = SDLGui_DoDialog(alertdlg, NULL, false);
+ i = SDLGui_DoDialog(alertdlg);
SDL_UpdateRect(sdlscrn, 0,0, 0,0);
SDL_ShowCursor(bOldMouseVisibility);
diff --git a/src/gui-sdl/dlgCpu.c b/src/gui-sdl/dlgCpu.c
index 41d2776a..ca3777ea 100644
--- a/src/gui-sdl/dlgCpu.c
+++ b/src/gui-sdl/dlgCpu.c
@@ -146,7 +146,7 @@ void DlgCpu_Main(void)
cpudlg[DLGCPU_SOFTFLOAT].state &= ~SG_SELECTED;
/* Show the dialog: */
- SDLGui_DoDialog(cpudlg, NULL, false);
+ SDLGui_DoDialog(cpudlg);
/* Read values from dialog: */
diff --git a/src/gui-sdl/dlgDevice.c b/src/gui-sdl/dlgDevice.c
index 3988cd9e..5a5048e8 100644
--- a/src/gui-sdl/dlgDevice.c
+++ b/src/gui-sdl/dlgDevice.c
@@ -157,7 +157,7 @@ void Dialog_DeviceDlg(void)
/* The devices dialog main loop */
do
{
- but = SDLGui_DoDialog(devicedlg, NULL, false);
+ but = SDLGui_DoDialog(devicedlg);
switch(but)
{
diff --git a/src/gui-sdl/dlgFileSelect.c b/src/gui-sdl/dlgFileSelect.c
index 6954031e..da724f07 100644
--- a/src/gui-sdl/dlgFileSelect.c
+++ b/src/gui-sdl/dlgFileSelect.c
@@ -372,6 +372,18 @@ static void DlgFileSelect_ManageScrollbar(void)
refreshentries = true;
}
+/*-----------------------------------------------------------------------*/
+/**
+ * Return true for handled SDL events, should match what's
+ * handled in DlgFileSelect_HandleSdlEvents()
+ */
+static bool acceptEvents(SDL_EventType evtype)
+{
+ if (evtype == SDL_MOUSEWHEEL || evtype == SDL_KEYDOWN)
+ return true;
+ return false;
+}
+
/*-----------------------------------------------------------------------*/
/**
* Handle SDL events.
@@ -658,7 +670,6 @@ char* SDLGui_FileSelect(const char *title, const char *path_and_name, char **zip
char *mtxt;
const char *ctxt;
} dlgtitle; /* A hack to silent recent GCCs warnings */
- bool KeepCurrentObject;
dlgtitle.ctxt = title;
@@ -732,10 +743,8 @@ char* SDLGui_FileSelect(const char *title, const char *path_and_name, char **zip
refreshDrive(path[0]);
#endif
- /* The first time we display the dialog, we reset the current position */
- /* On next calls, current_object's value will be kept to handle scrolling */
- KeepCurrentObject = false;
-
+ /* current object when entering the dialog */
+ retbut = SDLGUI_NOTFOUND;
do
{
if (reloaddir)
@@ -812,8 +821,7 @@ char* SDLGui_FileSelect(const char *title, const char *path_and_name, char **zip
}
/* Show dialog: */
- retbut = SDLGui_DoDialog(fsdlg, &sdlEvent, KeepCurrentObject);
- KeepCurrentObject = true; /* Don't reset current_object for next calls */
+ retbut = SDLGui_DoDialogExt(fsdlg, retbut, acceptEvents, &sdlEvent);
/* Has the user clicked on a file or folder? */
if (retbut>=SGFSDLG_ENTRYFIRST && retbut<=SGFSDLG_ENTRYLAST && retbut-SGFSDLG_ENTRYFIRST+ypos<entries)
diff --git a/src/gui-sdl/dlgFloppy.c b/src/gui-sdl/dlgFloppy.c
index 889b0dca..faecf04c 100644
--- a/src/gui-sdl/dlgFloppy.c
+++ b/src/gui-sdl/dlgFloppy.c
@@ -155,7 +155,7 @@ static void DlgFloppy_QueryInsert(char *namea, int ida, char *nameb, int idb, co
char *dlgname;
SDLGui_CenterDlg(alertdlg);
- switch (SDLGui_DoDialog(alertdlg, NULL, false))
+ switch (SDLGui_DoDialog(alertdlg))
{
case DLGMOUNT_A:
dlgname = namea;
@@ -256,7 +256,7 @@ void DlgFloppy_Main(void)
/* Draw and process the dialog */
do
{
- but = SDLGui_DoDialog(floppydlg, NULL, false);
+ but = SDLGui_DoDialog(floppydlg);
switch (but)
{
case FLOPPYDLG_EJECTA: /* Eject disk in drive A: */
diff --git a/src/gui-sdl/dlgHalt.c b/src/gui-sdl/dlgHalt.c
index 896cf057..cbf4be87 100644
--- a/src/gui-sdl/dlgHalt.c
+++ b/src/gui-sdl/dlgHalt.c
@@ -73,7 +73,7 @@ void Dialog_HaltDlg(void)
if (SDLGui_SetScreen(sdlscrn))
return;
SDLGui_CenterDlg(haltdlg);
- switch (SDLGui_DoDialog(haltdlg, NULL, false)) {
+ switch (SDLGui_DoDialog(haltdlg)) {
case DLGHALT_WARM:
/* Reset to exit 'halt' state (resets CPU and regs.spcflags) */
diff --git a/src/gui-sdl/dlgHardDisk.c b/src/gui-sdl/dlgHardDisk.c
index 187de561..72f6dff4 100644
--- a/src/gui-sdl/dlgHardDisk.c
+++ b/src/gui-sdl/dlgHardDisk.c
@@ -254,7 +254,7 @@ void DlgHardDisk_Main(void)
/* Draw and process the dialog */
do
{
- but = SDLGui_DoDialog(diskdlg, NULL, false);
+ but = SDLGui_DoDialog(diskdlg);
switch (but)
{
case DISKDLG_ACSIPREVID:
diff --git a/src/gui-sdl/dlgJoystick.c b/src/gui-sdl/dlgJoystick.c
index da4965e4..341f63dd 100644
--- a/src/gui-sdl/dlgJoystick.c
+++ b/src/gui-sdl/dlgJoystick.c
@@ -226,7 +226,7 @@ void Dialog_JoyDlg(void)
do
{
- but = SDLGui_DoDialog(joydlg, NULL, false);
+ but = SDLGui_DoDialog(joydlg);
switch (but)
{
case DLGJOY_PREVSDLJOY: // Select the previous SDL joystick
diff --git a/src/gui-sdl/dlgKeyboard.c b/src/gui-sdl/dlgKeyboard.c
index 0e782a2d..f5d23770 100644
--- a/src/gui-sdl/dlgKeyboard.c
+++ b/src/gui-sdl/dlgKeyboard.c
@@ -245,7 +245,7 @@ void Dialog_KeyboardDlg(void)
/* Show the dialog: */
do
{
- but = SDLGui_DoDialog(keyboarddlg, NULL, false);
+ but = SDLGui_DoDialog(keyboarddlg);
switch (but)
{
diff --git a/src/gui-sdl/dlgMain.c b/src/gui-sdl/dlgMain.c
index 0e3c316c..e4eb9868 100644
--- a/src/gui-sdl/dlgMain.c
+++ b/src/gui-sdl/dlgMain.c
@@ -93,7 +93,7 @@ int Dialog_MainDlg(bool *bReset, bool *bLoadedSnapshot)
do
{
- retbut = SDLGui_DoDialog(maindlg, NULL, false);
+ retbut = SDLGui_DoDialog(maindlg);
switch (retbut)
{
case MAINDLG_ABOUT:
diff --git a/src/gui-sdl/dlgMemory.c b/src/gui-sdl/dlgMemory.c
index d81f5ddb..3b828861 100644
--- a/src/gui-sdl/dlgMemory.c
+++ b/src/gui-sdl/dlgMemory.c
@@ -143,7 +143,7 @@ bool Dialog_MemDlg(void)
do
{
- but = SDLGui_DoDialog(memorydlg, NULL, false);
+ but = SDLGui_DoDialog(memorydlg);
switch (but)
{
diff --git a/src/gui-sdl/dlgNewDisk.c b/src/gui-sdl/dlgNewDisk.c
index 5511cc51..8f10f83e 100644
--- a/src/gui-sdl/dlgNewDisk.c
+++ b/src/gui-sdl/dlgNewDisk.c
@@ -122,7 +122,7 @@ char *DlgNewDisk_Main(void)
/* Draw and process the dialog */
do
{
- but = SDLGui_DoDialog(newdiskdlg, NULL, false);
+ but = SDLGui_DoDialog(newdiskdlg);
switch(but)
{
case DLGNEWDISK_DECTRACK:
diff --git a/src/gui-sdl/dlgRom.c b/src/gui-sdl/dlgRom.c
index 20e73db6..9059af95 100644
--- a/src/gui-sdl/dlgRom.c
+++ b/src/gui-sdl/dlgRom.c
@@ -63,7 +63,7 @@ void DlgRom_Main(void)
do
{
- but = SDLGui_DoDialog(romdlg, NULL, false);
+ but = SDLGui_DoDialog(romdlg);
switch (but)
{
case DLGROM_TOSBROWSE:
diff --git a/src/gui-sdl/dlgScreen.c b/src/gui-sdl/dlgScreen.c
index fbc5491a..8cc97de0 100644
--- a/src/gui-sdl/dlgScreen.c
+++ b/src/gui-sdl/dlgScreen.c
@@ -239,7 +239,7 @@ void Dialog_MonitorDlg(void)
/* The monitor dialog main loop */
do
{
- but = SDLGui_DoDialog(monitordlg, NULL, false);
+ but = SDLGui_DoDialog(monitordlg);
switch (but)
{
case DLGSCRN_VDI_WLESS:
@@ -376,7 +376,7 @@ void Dialog_WindowDlg(void)
/* The window dialog main loop */
do
{
- but = SDLGui_DoDialog(windowdlg, NULL, false);
+ but = SDLGui_DoDialog(windowdlg);
switch (but)
{
case DLGSCRN_MAX_WLESS:
diff --git a/src/gui-sdl/dlgSound.c b/src/gui-sdl/dlgSound.c
index 87fc8ae5..cf844565 100644
--- a/src/gui-sdl/dlgSound.c
+++ b/src/gui-sdl/dlgSound.c
@@ -146,7 +146,7 @@ void Dialog_SoundDlg(void)
/* The sound dialog main loop */
do
{
- but = SDLGui_DoDialog(sounddlg, NULL, false);
+ but = SDLGui_DoDialog(sounddlg);
switch (but)
{
case DLGSOUND_RECBROWSE: /* Choose a new record file */
diff --git a/src/gui-sdl/dlgSystem.c b/src/gui-sdl/dlgSystem.c
index 8d29db0f..aa097b92 100644
--- a/src/gui-sdl/dlgSystem.c
+++ b/src/gui-sdl/dlgSystem.c
@@ -127,7 +127,7 @@ void DlgSystem_Main(void)
systemdlg[DLGSYS_FASTBOOT].state &= ~SG_SELECTED;
/* Show the dialog: */
- SDLGui_DoDialog(systemdlg, NULL, false);
+ SDLGui_DoDialog(systemdlg);
/* Read values from dialog: */
diff --git a/src/gui-sdl/sdlgui.c b/src/gui-sdl/sdlgui.c
index fe7b0dd5..77eb01bf 100644
--- a/src/gui-sdl/sdlgui.c
+++ b/src/gui-sdl/sdlgui.c
@@ -34,7 +34,6 @@ static SDL_Surface *pSdlGuiScrn; /* Pointer to the actual main SDL sc
static SDL_Surface *pSmallFontGfx = NULL; /* The small font graphics */
static SDL_Surface *pBigFontGfx = NULL; /* The big font graphics */
static SDL_Surface *pFontGfx = NULL; /* The actual font graphics */
-static int current_object = SDLGUI_NOTFOUND;/* Current selected object */
static struct {
Uint32 darkbar, midbar, lightbar;
@@ -1076,13 +1075,14 @@ static void SDLGui_ScaleMouseButtonCoordinates(SDL_MouseButtonEvent *bev)
/**
* Show and process a dialog. Returns either:
* - index of the GUI item that was invoked
- * - SDLGUI_UNKNOWNEVENT if an unsupported event occurred
- * (will be stored in parameter pEventOut)
* - SDLGUI_QUIT if user wants to close Hatari
* - SDLGUI_ERROR if unable to show dialog
+ * - for events not handled here, 'isEventOut' callback is checked
+ * for whether caller is interested about given event type:
+ * => event is stored to pEventOut and SDLGUI_UNKNOWNEVENT returned
* GUI item indices are positive, other return values are negative
*/
-int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
+int SDLGui_DoDialogExt(SGOBJ *dlg, int current_object, bool (*isEventOut)(SDL_EventType), SDL_Event *pEventOut)
{
int oldbutton = SDLGUI_NOTFOUND;
int retbutton = SDLGUI_NOTFOUND;
@@ -1094,12 +1094,8 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
SDL_Rect dlgrect, bgrect;
SDL_Joystick *joy = NULL;
- /* In the case of dialog using a scrollbar, we must keep the previous */
- /* value of current_object, as the same dialog is displayed in a loop */
- /* to handle scrolling. For other dialogs, we need to reset current_object */
- /* (ie no object selected at start when displaying the dialog) */
- if ( !KeepCurrentObject )
- current_object = 0;
+ /* either both, or neither of these should be present */
+ assert((isEventOut && pEventOut) || (!isEventOut && !pEventOut));
if (pSdlGuiScrn->h / sdlgui_fontheight < dlg[0].h)
{
@@ -1159,15 +1155,14 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
/* also if the mouse pointer has left the scrollbar */
if (current_object != SDLGUI_NOTFOUND && dlg[current_object].type == SGSCROLLBAR) {
obj = current_object;
- retbutton = obj;
oldbutton = obj;
if (b & SDL_BUTTON(1))
{
+ retbutton = obj;
dlg[obj].state |= SG_MOUSEDOWN;
}
else
{
- current_object = 0;
dlg[obj].state &= ~SG_MOUSEDOWN;
}
}
@@ -1179,8 +1174,8 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
oldbutton = obj;
if (b & SDL_BUTTON(1))
{
- dlg[obj].state |= SG_SELECTED;
retbutton = obj;
+ dlg[obj].state |= SG_SELECTED;
}
}
}
@@ -1205,8 +1200,7 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
if (sdlEvent.button.button != SDL_BUTTON_LEFT)
{
/* Not left mouse button -> unsupported event */
- if (pEventOut)
- retbutton = SDLGUI_UNKNOWNEVENT;
+ retbutton = SDLGUI_UNKNOWNEVENT;
break;
}
/* It was the left button: Find the object under the mouse cursor */
@@ -1239,8 +1233,7 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
if (sdlEvent.button.button != SDL_BUTTON_LEFT)
{
/* Not left mouse button -> unsupported event */
- if (pEventOut)
- retbutton = SDLGUI_UNKNOWNEVENT;
+ retbutton = SDLGUI_UNKNOWNEVENT;
break;
}
/* It was the left button: Find the object under the mouse cursor */
@@ -1333,7 +1326,7 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
if (key >= 33 && key <= 126)
retbutton = SDLGui_HandleShortcut(dlg, toupper(key));
}
- if (!retbutton && pEventOut)
+ if (!retbutton)
retbutton = SDLGUI_UNKNOWNEVENT;
break;
}
@@ -1367,8 +1360,7 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
retbutton = SDLGui_SearchFlags(dlg, SG_CANCEL);
break;
default:
- if (pEventOut)
- retbutton = SDLGUI_UNKNOWNEVENT;
+ retbutton = SDLGUI_UNKNOWNEVENT;
break;
}
break;
@@ -1383,13 +1375,24 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
break;
default:
- if (pEventOut)
- retbutton = SDLGUI_UNKNOWNEVENT;
+ retbutton = SDLGUI_UNKNOWNEVENT;
break;
}
+ /* continue if unknown events were not not requested
+ * specifically for this event type
+ */
+ if (retbutton == SDLGUI_UNKNOWNEVENT &&
+ !(isEventOut && isEventOut(sdlEvent.type)))
+ retbutton = SDLGUI_NOTFOUND;
}
}
+ /* Copy event data of unsupported events if caller wants to have it */
+ if (retbutton == SDLGUI_UNKNOWNEVENT)
+ {
+ memcpy(pEventOut, &sdlEvent, sizeof(SDL_Event));
+ }
+
/* Restore background */
if (pBgSurface)
{
@@ -1397,16 +1400,25 @@ int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject)
SDL_FreeSurface(pBgSurface);
}
- /* Copy event data of unsupported events if caller wants to have it */
- if (retbutton == SDLGUI_UNKNOWNEVENT && pEventOut)
- memcpy(pEventOut, &sdlEvent, sizeof(SDL_Event));
-
if (retbutton == SDLGUI_QUIT)
bQuitProgram = true;
if (joy)
SDL_JoystickClose(joy);
- Dprintf(("EXIT - ret: %d, current: %d\n", retbutton, current_object));
+ Dprintf(("EXIT - ret: %d\n", retbutton));
return retbutton;
}
+
+/*-----------------------------------------------------------------------*/
+/**
+ * Show and process a dialog. Returns either:
+ * - index of the GUI item that was invoked
+ * - SDLGUI_QUIT if user wants to close Hatari
+ * - SDLGUI_ERROR if unable to show dialog
+ * GUI item indices are positive, other return values are negative
+ */
+int SDLGui_DoDialog(SGOBJ *dlg)
+{
+ return SDLGui_DoDialogExt(dlg, 0, NULL, NULL);
+}
diff --git a/src/includes/sdlgui.h b/src/includes/sdlgui.h
index b69040ab..9244d8f0 100644
--- a/src/includes/sdlgui.h
+++ b/src/includes/sdlgui.h
@@ -80,7 +80,8 @@ extern int SDLGui_SetScreen(SDL_Surface *pScrn);
extern void SDLGui_GetFontSize(int *width, int *height);
extern void SDLGui_Text(int x, int y, const char *txt);
extern void SDLGui_DrawDialog(const SGOBJ *dlg);
-extern int SDLGui_DoDialog(SGOBJ *dlg, SDL_Event *pEventOut, bool KeepCurrentObject);
+extern int SDLGui_DoDialog(SGOBJ *dlg);
+extern int SDLGui_DoDialogExt(SGOBJ *dlg, int objidx, bool (*isEventOut)(SDL_EventType), SDL_Event *pEventOut);
extern void SDLGui_CenterDlg(SGOBJ *dlg);
extern char* SDLGui_FileSelect(const char *title, const char *path_and_name, char **zip_path, bool bAllowNew);
extern bool SDLGui_FileConfSelect(const char *title, char *dlgname, char *confname, int maxlen, bool bAllowNew);
--
2.20.1
--------------F970788BD10315A1F956DF04--