Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add main_menu_style setting #6737

Closed
wants to merge 5 commits into from
Closed

Conversation

BeeeWall
Copy link
Contributor

@BeeeWall BeeeWall commented Dec 5, 2017

Same as #6080, but using a branch besides master, and without the annoying (unintentional) reversion commit.
This adds a setting to change between the simple (currently Android) UI and the full computer UI. By default, Android uses the simple menu, while other platforms use the full one. (If there is a way to detect screen size from Minetest, I'd rather base the default on that than the platform, but I don't know of a way to do that.)

if PLATFORM == "Android" then
menustyle = "simple"
else
menustyle = full
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be the string "full" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes, thanks

@paramat paramat added @ Mainmenu Feature ✨ PRs that add or enhance a feature labels Dec 5, 2017
menustyle = "simple"
else
menustyle = "full"
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

menustyle = PLATFORM == "Android" and "simple" or "full"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about that, thanks.

@@ -24,6 +24,15 @@ mt_color_dark_green = "#25C191"

local menupath = core.get_mainmenu_path()
local basepath = core.get_builtin_path()
local menustylesetting = core.settings:get("main_menu_style")
local menustyle = menustylesetting
Copy link
Member

@SmallJoker SmallJoker Dec 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local menustyle = core.settings:get("main_menu_style"), superfluous menustylesetting
EDIT:

local menustyle = core.settings:get("main_menu_style") or "default"

As there's no entry for the default value in defaultsettings.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If menustylesetting is set to "default", then menustyle depends on the platform. Should I just use menustyle and then just redefine it?

@@ -24,6 +24,11 @@ mt_color_dark_green = "#25C191"

local menupath = core.get_mainmenu_path()
local basepath = core.get_builtin_path()
local menustyle = core.settings:get("main_menu_style")
local menustyle = menustylesetting
if menustyle == "default" then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local menustyle = core.settings:get("main_menu_style") or "default"
if menustyle == "default" then
	menustyle = PLATFORM == "Android" and "simple" or "full"
end

Copy link
Contributor Author

@BeeeWall BeeeWall Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I could swear I had changed this in my last commit. Will do ASAP.
Also, what does the or "default" do? In settingtypes.txt, I set the default to the value default, so I don't think that should be needed. But this is the only lua I've ever written, so I don't know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core.settings:get does not care about settingtypes.txt. That file is only used (yet) to display accurate information in the Advanced Settings dialog.

Copy link
Contributor Author

@BeeeWall BeeeWall Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok. Where does it check? I've been looking at the code, but I'm having trouble figuring it out. Do I just need to add it to defaultsettings.cpp?
Also, in minetest/src/script/lua_api/l_mainmenu.cpp, I see lua_pushstring(L,"display_width"); and lua_pushstring(L,"display_height");. Are these available from lua then, because I've been looking for a way to check screen size rather than platform, for tablets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed menustylesetting reference, added to defaultsettings.cpp

@ghost
Copy link

ghost commented Dec 28, 2017

This is needed for Windows 10 laptops that toggle between Desktop and Tablet modes. It's the first half, and then the second half are the touchscreen controls.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works.

@paramat
Copy link
Contributor

paramat commented Dec 28, 2017

Why would Android UI be needed on a laptop size screen?
I can't imagine anyone choosing touchscreen controls over the already present keyboard, so i don't think optional touchscreen controls are needed at all.

EDIT: Misunderstood.

@ThomasMonroe314
Copy link
Contributor

@paramat maybe an android tablet?

@paramat
Copy link
Contributor

paramat commented Dec 28, 2017

Above i'm referring to a switchable laptop, i know this is useful for large-screen tablets.

@ghost
Copy link

ghost commented Dec 28, 2017 via email

@BeeeWall
Copy link
Contributor Author

Because there's no keyboard

Even if you wanted touchscreen, you'd still likely want the full menu. Simple is just for small devices. In fact, I've been trying to find some way to set the default based on screen size rather than platform, but I'm having issues finding needed lua variables to do so.

@paramat
Copy link
Contributor

paramat commented Dec 29, 2017

I misunderstood.

@BeeeWall
Copy link
Contributor Author

In minetest/src/script/lua_api/l_mainmenu.cpp, there's the lines lua_pushstring(L,"display_width"); and lua_pushstring(L,"display_height");. Does that mean that there are display_height and display_width variables I could use in Lua, and would these be resolution (in pixels, I'd guess), or screen size (inches or cm or something)?

@sfan5
Copy link
Member

sfan5 commented Feb 8, 2018

0e4c467

@sfan5 sfan5 closed this Feb 8, 2018
@paramat
Copy link
Contributor

paramat commented Feb 8, 2018

👍 from sfan5, implied by merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants