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

Always use same default tabheader height #9319

Merged
merged 1 commit into from May 9, 2020

Conversation

p-ouellette
Copy link
Contributor

@p-ouellette p-ouellette commented Jan 19, 2020

Previously the default tabheader height was different when using
real coordinates. This resulted in the height of tabs changing when
switching tabs in sfinv if some tabs used real coordinates.

To do

This PR is a Ready for Review.

How to test

Apply the patch below to sfinv. Open sfinv and switch between the crafting tab and other tabs (ignore the messed up contents of the crafting tab).
Before this PR: tabheader height changes when switching between crafting tab and other tabs.
After: tabheader hight unchanged between tabs.

diff --git a/mods/sfinv/init.lua b/mods/sfinv/init.lua
index 71e9ee7..47424ea 100644
--- a/mods/sfinv/init.lua
+++ b/mods/sfinv/init.lua
@@ -14,6 +14,6 @@ sfinv.register_page("sfinv:crafting", {
                                image[4.75,1.5;1,1;sfinv_crafting_arrow.png]
                                listring[current_player;main]
                                listring[current_player;craft]
-                       ]], true)
+                       ]], true, "formspec_version[3]size[10.5,11.375]")
        end
 })

Note: the form width of 10.5 comes from the conversion formula in lua_api.txt, but that formula did not give the correct height (I arrived at 11.375 by guess and check, the formula suggested 11.875). Not sure what's going on there.

@sfan5 sfan5 added Bugfix 🐛 PRs that fix a bug Formspec Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Jan 19, 2020
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@rubenwardy rubenwardy self-requested a review April 4, 2020 16:24
@rubenwardy
Copy link
Member

rubenwardy commented Apr 4, 2020

This code is valid and works, but I'm trying to think of a better way to structure it. Having the string -> float conversion in v_geom is annoying to my logic. This is as far I got:

// Work out the formspec-coordinate size, if in real-coordinates mode
bool auto_width = true;
std::vector<std::string> v_geom = {"1", "1"}; // Dummy width and height
if (data->real_coordinates) {
	if (parts.size() == 7) {
		i++;

		v_geom = split(parts[1], ',');
		if (v_geom.size() == 1)
			v_geom.insert(v_geom.begin(), std::to_string(2.f * (float)m_btn_height / (float)imgsize.Y));
		else
			auto_width = false;
	} else {
		v_geom[1] = std::to_string(2.f * (float)m_btn_height / (float)imgsize.Y);
	}
}

I was attempting to get the formspec parsing done in one place, so that the formspec coord -> irrlicht coord conversion part wouldn't care about it

@rubenwardy
Copy link
Member

rubenwardy commented Apr 4, 2020

perhaps:

// Work out the formspec-coordinate size, if in real-coordinates mode
const auto default_height = std::to_string(2.f * (float)m_btn_height / (float)imgsize.Y);
bool auto_width = true;
std::vector<std::string> v_geom = {"1", default_height};
if (parts.size() == 7) {
	i++;

	v_geom = split(parts[1], ',');
	if (v_geom.size() == 1)
		v_geom.insert(v_geom.begin(), default_height);
	else
		auto_width = false;
}

@rubenwardy rubenwardy modified the milestone: 5.2.0 Apr 4, 2020
Previously the default tabheader height was different when using
real coordinates. This resulted in the height of tabs changing when
switching tabs in sfinv if some tabs used real coordinates.
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

@SmallJoker SmallJoker merged commit b624249 into minetest:master May 9, 2020
@p-ouellette p-ouellette deleted the default-tabheader-height branch May 9, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug Formspec Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants