Replace Irrlicht file picker with new Lua dialog #13512

rollerozxa · 2023-05-16T18:43:20Z

Based on #10580, this PR replaces the current Irrlicht file/folder picker with a new one written in Lua. The code for the old Irrlicht picker dialog on the Minetest engine side of things has also been removed.

Fixes #8073

To do

This PR is a Ready for Review.

How to test

Test with a setting expecting a filepath (e.g. font_path). Check so that you can only pick files, not folders.
Test with a setting expecting a (folder)path (e.g. shader_path). Check so that you can only pick folders, not folders.

Platforms:

Linux: Works for me
Android: Barely functions due to it not being possible to double tap a formspec table (but does work with a keyboard, haha). Not a regression from the old Irrlicht dialog though, which straight out wouldn't function on Android.
Windows: Needs testing.

sfan5 · 2023-05-17T15:03:10Z

Note that you need to make sure to appropriately preserve the commit author information.

Bituvo · 2023-05-20T20:51:03Z

Why can't we just use the system file/folder picker?

oong819 · 2023-05-21T06:21:34Z

i think Minetest should use SAF file picker on Android, it's from Android system so we dont need to worry much about maintence

SmallJoker · 2023-05-21T07:54:57Z

The system file picker that you refer to might originate from GTK on Linux, comdlg32.dll on Windows and whatever else on macOS. Whereas this could be used, it would at least introduce compile-time dependencies on Linux and platform-specific implementations to handle the returned value. This is less convenient than reinventing the wheel using existing file listing functions and formspecs.

srifqi · 2023-05-25T03:30:00Z

I tested this PR on Windows 10 (10.0.19044) and found problems with path separator. Windows uses backslash (\) as its path separator. The dialog shows no path separator. The other problem is that I can not go to root of a drive or change drive.

When I entered C:\git\minetest\ in the address bar, the address bar disappeared and the file list is broken.

Before this PR

The other problem (related but not affected by this PR) is that the problem also happens in the new setting dialog.

The path is C:\git\minetest\ to the root folder.

Co-authored-by: Jean-Patrick Guerrero <jeanpatrick.guerrero@gmail.com>

rollerozxa · 2023-05-29T21:04:39Z

Note that you need to make sure to appropriately preserve the commit author information.

I assume I would add kilbith as a co-author to the commit?

(testing filebrowser on windows)

Yikes, looks like it really doesn't like Windows paths. I've done some changes which makes it work for me under Wine. Please retest.

(new settings dialog on windows)

Looks like the new settings dialog value doesn't escape the value for file paths. Fixed that as well since I'm already making changes around that area anyways.

sfan5 · 2023-05-29T21:09:16Z

I assume I would add kilbith as a co-author to the commit?

I expected kilbith as commit author and you as co-author. I suppose either way is fine actually, we still have the author info the file itself.

srifqi · 2023-05-29T23:00:16Z

It is working now on Windows.

I wonder if it would be easy to get the drive list on Windows (not necessary though since we can enter the path manually).

(EDIT: probably not related)

I want to ask about the default setting behaviour for path. Is it intended? The default value (when set to it) still works, though.

rollerozxa · 2023-05-31T10:05:50Z

I want to ask about the default setting behaviour for path. Is it intended? The default value (when set to it) still works, though.

That's just a flaw with reading default values from settingtypes.txt, it can differ from the default values provided by the engine in defaultsettings.cpp. This can be especially noticeable for settings that have different defaults on Android, the settings menu will let you reset them to the desktop counterpart (see e.g. contentdb_flag_blacklist). The default font settings are always set to be absolute, leading them to always being considered "dirty" or changed from the generic path provided in settingtypes.txt no matter the platform.

Exposing the default settings values to the main menu so the settings dialog can read from them too (overriding settingtypes.txt's defaults) would be the fix, I guess, but that's for another PR or an issue to be opened about this.

grorp · 2023-05-31T10:51:30Z

That's just a flaw with reading default values from settingtypes.txt, it can differ from the default values provided by the engine in defaultsettings.cpp. This can be especially noticeable for settings that have different defaults on Android, the settings menu will let you reset them to the desktop counterpart (see e.g. contentdb_flag_blacklist). The default font settings are always set to be absolute, leading them to always being considered "dirty" or changed from the generic path provided in settingtypes.txt no matter the platform.

Exposing the default settings values to the main menu so the settings dialog can read from them too (overriding settingtypes.txt's defaults) would be the fix, I guess, but that's for another PR or an issue to be opened about this.

This issue is (mostly) already fixed by #13489. The default values shown in the tooltips are still from settingtypes.txt, but the "detect if the setting is dirty" and the "reset the setting to its default value" code don't use the default values from settingtypes.txt anymore.

srifqi

I have tested it and it works. The concept and its implementation look good, except some code style issues in a few lines.

builtin/mainmenu/settings/dlg_file_browser.lua

sfan5 · 2023-06-06T15:32:30Z

builtin/mainmenu/settings/dlg_file_browser.lua

+	local dirs = get_dirs()
+
+	if fields.dirs then
+		local event, idx = fields.dirs:sub(1,3), tonumber(fields.dirs:match("%d+"))


didn't we have a helper to split events?

sfan5 · 2023-06-06T15:32:33Z

builtin/mainmenu/settings/dlg_file_browser.lua

+		end
+	end
+
+	local dirs = get_dirs()


this shouldn't unconditionally be called
it's also prone to issues if the dir list changes between load time and click time
cache this in dialogdata?

grorp

I couldn't resist ¯_(ツ)_/¯

grorp · 2023-06-06T17:32:36Z

builtin/mainmenu/settings/dlg_file_browser.lua

+	end
+
+	for _, f in ipairs(dirs) do
+		local is_file = not core.is_dir(PATH .. DIR_DELIM .. f)


The filename in f is already formspec-escaped.

Uh... yeah?

The point is escaped strings are not valid filesystem paths (anymore)

E.g. if you have a folder called $hello$ , it's shown as a file.

this is not fixed.

builtin/mainmenu/settings/dlg_file_browser.lua

Desour

Haven't looked through everything.

Fixes #8073, if I'm not mistaken.

LICENSE.txt

rollerozxa · 2023-06-25T18:23:21Z

I thought that since the file picker doesn't really work on Android due to the touchscreen interface not supporting doubleclicking on a table, should this be disabled on Android? The filepath and path setting types would be displayed as just a simple string, removing the browse button:

if PLATFORM == "Android" then
	make.filepath = make.string
	make.path = make.string
end

srifqi · 2023-07-12T00:00:04Z

For Android, the option is either (1) adding a button for each row of files in the dialog or (2) showing the setting as a text input. I prefer option (2) since the player needs external tools/apps to modify Minetest's folder (the player needs to be experienced in Android's file management system) and the player can just copy the file's path.

rubenwardy · 2023-07-12T09:34:59Z

I support just showing a string input on Android

…string field

sfan5 · 2023-07-30T16:17:11Z

builtin/mainmenu/settings/dlg_file_browser.lua

+-- 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+local PATH = os.getenv("HOME") -- Linux & similar
+		or os.getenv("HOMEDRIVE") .. os.getenv("HOMEPATH") -- Windows


probably have to do this to fix the issue @grorp mentioned

Suggested change

or os.getenv("HOMEDRIVE") .. os.getenv("HOMEPATH") -- Windows

or (os.getenv("HOMEDRIVE") .. os.getenv("HOMEPATH")) -- Windows

sfan5 · 2023-07-30T16:18:34Z

builtin/mainmenu/settings/dlg_file_browser.lua

+	end
+
+	for _, f in ipairs(dirs) do
+		local is_file = not core.is_dir(PATH .. DIR_DELIM .. f)


this is not fixed.

sfan5 · 2023-07-30T16:19:20Z

builtin/mainmenu/settings/dlg_file_browser.lua

+		end
+	end
+
+	local dirs = get_dirs()


Nukiloco · 2023-08-19T04:37:04Z

There should be an option to allow people to use the system file picker. Some people like their file managers more than built in ones since they are used to or love features included with their own systems. Some people even use terminal file pickers. This change may limit what users can use to program minetest games in some way.

sfan5 · 2023-08-19T10:27:06Z

This change doesn't limit anything, it was never possible to use the system file picker. Allowing that would require various OS-specific integration work when time is better spent elsewhere (IMO).

rubenwardy · 2023-08-19T10:39:41Z

In the past, I used a small library called tinyfiledialogs to use system file dialogs, bundled in the repo. I never had many bugs reported with it, but this sort of thing is likely to have portability issues and future maintenance cost

https://github.com/rollerozxa/NodeBoxEditor/blob/master/src/util/tinyfiledialogs.h

Desour added the @ Mainmenu label May 16, 2023

Zughy added Roadmap The change matches an item on the current roadmap. UI/UX Feature ✨ PRs that add or enhance a feature labels May 16, 2023

sfan5 self-requested a review May 17, 2023 14:58

Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 17, 2023

Replace Irrlicht file picker with new Lua dialog

1747f68

Co-authored-by: Jean-Patrick Guerrero <jeanpatrick.guerrero@gmail.com>

rollerozxa force-pushed the lua-filepicker branch from 8930e37 to b9c1216 Compare May 29, 2023 21:03

Fixes for Windows

22cfd93

rollerozxa force-pushed the lua-filepicker branch from b9c1216 to 22cfd93 Compare May 29, 2023 21:05

sfan5 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 30, 2023

srifqi approved these changes Jun 6, 2023

View reviewed changes

builtin/mainmenu/settings/dlg_file_browser.lua Outdated Show resolved Hide resolved

builtin/mainmenu/settings/dlg_file_browser.lua Outdated Show resolved Hide resolved

builtin/mainmenu/settings/dlg_file_browser.lua Outdated Show resolved Hide resolved

srifqi added the One approval ✅ ◻️ label Jun 6, 2023

sfan5 requested changes Jun 6, 2023

View reviewed changes

grorp reviewed Jun 6, 2023

View reviewed changes

Desour reviewed Jun 6, 2023

View reviewed changes

LICENSE.txt Show resolved Hide resolved

sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 10, 2023

corrections

d96e0e1

Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jul 10, 2023

sfan5 removed the One approval ✅ ◻️ label Jul 22, 2023

sfan5 self-requested a review July 22, 2023 15:13

rollerozxa added 2 commits July 30, 2023 17:03

Disable file browser dialog on Android, change path fields to simple …

c97723a

…string field

Add attribution for up_icon.png

409d082

sfan5 reviewed Jul 30, 2023

View reviewed changes

sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jul 30, 2023

rollerozxa mentioned this pull request Aug 5, 2023

Settings GUI: Fix path settings on Windows #13715

Merged

Zughy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Sep 12, 2023

Zughy closed this Sep 12, 2023

sfan5 mentioned this pull request Oct 13, 2023

Fix character encoding conversion issues minetest/irrlicht#249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Irrlicht file picker with new Lua dialog #13512

Replace Irrlicht file picker with new Lua dialog #13512

rollerozxa commented May 16, 2023 •

edited by rubenwardy

sfan5 commented May 17, 2023

Bituvo commented May 20, 2023

oong819 commented May 21, 2023

SmallJoker commented May 21, 2023 •

edited

srifqi commented May 25, 2023

rollerozxa commented May 29, 2023

sfan5 commented May 29, 2023 •

edited

srifqi commented May 29, 2023 •

edited

rollerozxa commented May 31, 2023

grorp commented May 31, 2023

srifqi left a comment

sfan5 Jun 6, 2023

sfan5 Jun 6, 2023

sfan5 Jul 30, 2023

grorp left a comment

grorp Jun 6, 2023

rollerozxa Jun 25, 2023

sfan5 Jun 25, 2023 •

edited

grorp Jun 25, 2023

sfan5 Jul 30, 2023

Desour left a comment

rollerozxa commented Jun 25, 2023

srifqi commented Jul 12, 2023

rubenwardy commented Jul 12, 2023

sfan5 Jul 30, 2023

sfan5 Jul 30, 2023

sfan5 Jul 30, 2023

Nukiloco commented Aug 19, 2023

sfan5 commented Aug 19, 2023

rubenwardy commented Aug 19, 2023

	or os.getenv("HOMEDRIVE") .. os.getenv("HOMEPATH") -- Windows
	or (os.getenv("HOMEDRIVE") .. os.getenv("HOMEPATH")) -- Windows

Replace Irrlicht file picker with new Lua dialog #13512

Replace Irrlicht file picker with new Lua dialog #13512

Conversation

rollerozxa commented May 16, 2023 • edited by rubenwardy

To do

How to test

sfan5 commented May 17, 2023

Bituvo commented May 20, 2023

oong819 commented May 21, 2023

SmallJoker commented May 21, 2023 • edited

srifqi commented May 25, 2023

rollerozxa commented May 29, 2023

sfan5 commented May 29, 2023 • edited

srifqi commented May 29, 2023 • edited

rollerozxa commented May 31, 2023

grorp commented May 31, 2023

srifqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 Jun 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Desour left a comment

Choose a reason for hiding this comment

rollerozxa commented Jun 25, 2023

srifqi commented Jul 12, 2023

rubenwardy commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nukiloco commented Aug 19, 2023

sfan5 commented Aug 19, 2023

rubenwardy commented Aug 19, 2023

rollerozxa commented May 16, 2023 •

edited by rubenwardy

SmallJoker commented May 21, 2023 •

edited

sfan5 commented May 29, 2023 •

edited

srifqi commented May 29, 2023 •

edited

sfan5 Jun 25, 2023 •

edited