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

textarea[], field[]: Unify function, fix wrong fallback text #6787

Merged
merged 4 commits into from Mar 9, 2018

Conversation

Projects
None yet
6 participants
@SmallJoker
Copy link
Member

commented Dec 14, 2017

Fixes minetest/minetest_game#1978

Corrects the compatibility code after #6510 and the suboptimal fix 6b23cab.
Instructions:
Use this test mod. If it shows as expected, it's all ok.

core.register_on_joinplayer(function(player)
	core.after(2, function(player_name)
	core.show_formspec(player_name, "lab:testy",
		"size[9,7]"..
		-- Book edit
		"field[0.5,1;3.5,0;title;Title:;" ..
			core.formspec_escape("Taking the hobbits to Isengard") .. "]" ..
		"textarea[0.5,1.5;3.5,4;text;Contents:;" ..
			core.formspec_escape("The hobbits the hobbits\nto Isengard") .. "]" ..
		-- Empty book
		"field[4.5,1;3.5,0;title2;Title:;]" ..
		"textarea[0.5,5.5;3.5,4;text2;Contents:;]" ..
		-- Server description (pre e6e5fa3, 0.4.16 stable)
		"textarea[4,2.3;4.23,2.9;;" ..
[[Blinded by the light
Revved up like a deuce
Another runner in the night
Blinded by the light

Madman drummer bummers
Indians in the summer
With a teenage diplomat
In the dumps with the mumps
As the adolescent pumps]] .. ";]" ..
		-- Readonly textarea with title
		"textarea[4.6,5.85;3.7,1.5;;This is a fancy title" ..
		";Never gonna give you up, never gonna let you down]"
	)
	end, player:get_player_name())
end)

EDIT
As of 180121 this PR also fixes editing of read-only text fields. (thanks to @red-001 for reporting)

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

Any chance that #5361 could get in by this occasion?

@SmallJoker

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2017

@kilbith I'd like to but that's out of scope of this PR, which already contains re-structured code and a bugfix.

@numberZero
Copy link
Contributor

left a comment

Works fine.

#if USE_FREETYPE && IRRLICHT_VERSION_MAJOR == 1 && IRRLICHT_VERSION_MINOR < 9
if (g_settings->getBool("freetype")) {
e = (gui::IGUIEditBox *) new gui::intlGUIEditBox(spec.fdefault.c_str(),
true, Environment, this, spec.fid, rect, is_editable, true);

This comment has been minimized.

Copy link
@numberZero

numberZero Dec 19, 2017

Contributor

has_vscrollbar is forced to be true. Is that intentional?
Also the cast could be implicit.

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Dec 19, 2017

Author Member

Not intentional. Will change to is_multiline, also switching to the implicit cast. Thanks for pointing this out.


gui::IGUIEditBox *e = nullptr;
#if USE_FREETYPE && IRRLICHT_VERSION_MAJOR == 1 && IRRLICHT_VERSION_MINOR < 9
if (g_settings->getBool("freetype")) {

This comment has been minimized.

Copy link
@numberZero

numberZero Dec 19, 2017

Contributor

There must be a cleaner way.

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Dec 19, 2017

Author Member

This is a piece of the old code. What cleaner way do you have in mind?

This comment has been minimized.

Copy link
@numberZero

numberZero Dec 19, 2017

Contributor

Something like

constexpr bool use_intl_edit_box = USE_FREETYPE && IRRLICHT_VERSION_MAJOR == 1 && IRRLICHT_VERSION_MINOR < 9;
...
if (use_intl_edit_box && g_settings->getBool("freetype"))

The include should not be guarded by that #if in this case, of course.
Unsure will my example compile reliably in all cases, but anyway, the current intermix of if and #if is too awful to be kept.

if (g_settings->getBool("freetype")) {
e = (gui::IGUIEditBox *) new gui::intlGUIEditBox(spec.fdefault.c_str(),
true, Environment, this, spec.fid, rect, is_editable, true);
e->drop();

This comment has been minimized.

Copy link
@numberZero

numberZero Dec 19, 2017

Contributor

Dropping before using looks dangerous. I suppose e is grabbed by the environment, but still, that looks like a bad practice.

this, spec.fid);
}

if (e != nullptr) {

This comment has been minimized.

Copy link
@numberZero

numberZero Dec 19, 2017

Contributor

if (e) {
Or
if (is_multiline || is_editable) {
as it can’t be NULL otherwise, I suppose. If so, the whole clause is extraneous and can be removed with replacing else at 1029 with else if (is_editable) or if (is_editable && !is_multiline) (the latter seems even cleaner in this particular case IMO)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Dec 19, 2017

Author Member

Environment->addEditBox may return NULL

This comment has been minimized.

Copy link
@numberZero

numberZero Dec 19, 2017

Contributor

Exception should probably be thrown in that case; that would be consistent with new throwing on failure.

@SmallJoker

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@numberZero Thanks for reviewing. I adapted your suggestions.

@numberZero

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

The include should not be guarded by that #if in this case, of course.

https://github.com/SmallJoker/minetest/blob/9f5b7080598fa6fd226b981d212ed2bfd60a7fb5/src/gui/guiFormSpecMenu.cpp#L59
Otherwise, it won’t compile without FreeType. I don’t know why was the #include guarded, that file obviously doesn’t depend on FreeType. The implementation doesn’t seem to depend on it as well. The fact that it might be useless without FreeType doesn’t mean it should be disabled that hardly.

@SmallJoker SmallJoker force-pushed the SmallJoker:textmess branch from 9f5b708 to b7a745b Dec 19, 2017

@SmallJoker SmallJoker force-pushed the SmallJoker:textmess branch from b7a745b to 85a4d5e Jan 11, 2018

@SmallJoker

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2018

Ready for reviewing. Rebased.

@SmallJoker SmallJoker force-pushed the SmallJoker:textmess branch from 85a4d5e to 79e0d8b Jan 21, 2018

@SmallJoker SmallJoker force-pushed the SmallJoker:textmess branch from 79e0d8b to fb401a5 Jan 21, 2018

@SmallJoker

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2018

Rebased again, added testing instructions.

@SmallJoker SmallJoker force-pushed the SmallJoker:textmess branch from cd63d95 to 27ab47d Jan 21, 2018

@SmallJoker SmallJoker force-pushed the SmallJoker:textmess branch from 27ab47d to 8ed3f13 Jan 21, 2018

spec.send = true;

// Multiline textareas: swap default and label for backwards compat
if (!is_editable && is_multiline &&

This comment has been minimized.

Copy link
@nerzhul

nerzhul Jan 22, 2018

Member

else if (and remove !is_editable)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jan 22, 2018

Author Member

Done. Also added technically superfluous brackets to keep the comment line

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

Time to get moving?

@nerzhul

nerzhul approved these changes Mar 9, 2018

@nerzhul nerzhul added the One approval label Mar 9, 2018

@rubenwardy rubenwardy merged commit 473d81f into minetest:master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.