Enhance: add Lua API for map symbol font #4038

SlySven · 2020-08-22T21:51:22Z

Adds setupMapSymbolFont((string) fontName[, (bool) onlyUseThisFont[, (float) scaling factor]])
and mapSymbolFontInfo().

The former takes:

a font name (default is currently: "Bitstream Vera Sans Mono" which can be also be specified with an EMPTY {i.e. ""} string. Alternatively, should the other arguments want to be set but not this one a nil is also acceptable for this argument to not change the font.
whether to only use the specified font or to use any that is available, use true or false to force the setting, or if it is nil (or not provided) no change is made so that the third argument can still be provided.
the scaling factor (between 0.50 to 2.00) that can be used to tweak the scale (default is 1.00) compared to the calculated font size that should enable an 'M' to fit inside the room rectangle or circle.

The latter returns a table with the following elements:

(string) the (family) "fontName"
(bool) "onlyUseThisFont"
(float) "scalingFactor"

Note that both of this functions operate only when a map is loaded as the map symbol font details are store in the map file (and NOT in the profile).

This PR should close #4031.

Signed-off-by: Stephen Lyons slysven@virginmedia.com

Adds `mapSymbolFontInfo((string) fontName[, (bool) onlyUseThisFont[, (float) scaling factor]])` and `mapSymbolFontInfo()`. The former takes: * a font name (default is currently: "Bitstream Vera Sans Mono" which can be also be specified with an EMPTY {i.e. `""`} string. Alternatively, should the other arguments want to be set but not this one a `nil` is also acceptable for this argument to not change the font. * whether to only use the specified font or to use any that is available, use `true` or `false` to force the setting, or if it is `nil` (or not provided) no change is made so that the third argument can still be provided. * the scaling factor (between 0.50 to 2.00) that can be used to tweak the scale (default is 1.00) compared to the calculated font size that should enable an 'M' to fit inside the room rectangle or circle. The latter returns a table with the following elements: * (string) the (family) "fontName" * (bool) "onlyUseThisFont" * (float) "scalingFactor" Note that both of this functions operate only when a map is loaded as the map symbol font details are store in the map file (and NOT in the profile). This PR should close Mudlet#4031. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add-deployment-links · 2020-08-22T21:51:47Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

vadi2 · 2020-08-22T23:03:22Z

👍 looks good at a glance, not able to test to approve.

tdk1069 · 2020-08-23T16:53:26Z

Just been playing with this, Including the Gameicon font with mpackage, I was able to change the default map font on install without any issue for me

vadi2

Looks good, can you flip the boolean so if you don't supply it, and the requested font isn't present, the nearest requested is used instead? We aren't a medical device here, so a better experience by default wins over precision.

vadi2 · 2020-09-13T12:21:22Z

src/TLuaInterpreter.cpp

@@ -3680,6 +3680,107 @@ int TLuaInterpreter::setMapWindowTitle(lua_State* L)
    return 1;
 }

+// Documentation: https://wiki.mudlet.org/w/Manual:Lua_Functions#setupMapSymbolFont
+int TLuaInterpreter::setupMapSymbolFont(lua_State* L)


Suggested change

int TLuaInterpreter::setupMapSymbolFont(lua_State* L)

int TLuaInterpreter::setMapSymbolFont(lua_State* L)

Just so it's consistent with other calls

Actually I intentionally used "setup" because when you are messing with fonts it isn't always as simple as a set - as you don't always get what you ask for (AFAICT) - instead the underlying Qt library takes a look at what you asked for and sees if it can come up with something (what it decides actually goes into a QFontInfo class - how well that matches to the original QFont can vary from system to system even with the same OS as they will not have the same fonts installed in the same order in the same locations.)

I know, but the fine difference will be lost on people and consistency is better.

vadi2 · 2020-09-13T12:21:58Z

src/TLuaInterpreter.cpp

+    QString fontName;
+    int s = 0;
+    if (!(lua_isnil(L, ++s) || lua_isstring(L, s))) {
+        lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (symbol font name as string is optional {use nil for no change, empty string for default}, got %s!)", s, luaL_typename(L, s));


Suggested change

lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (symbol font name as string is optional {use nil for no change, empty string for default}, got %s!)", s, luaL_typename(L, s));

lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (symbol font name as string is optional, got %s!)", s, luaL_typename(L, s));

Too much information, hard to understand it all when you're new to this, this way is better.

But it is an important functional difference.

If you need to alter one of the other following parameters but not change this setting you need to use a nil here as a placeholder. Whereas using the empty string "" is an explicit expression that the value is to be set to something - which has the meaning of whatever the default is (Deja Vu Mono IIRC). Mixing them up could produce unwanted results (changing back to the default instead of leaving the current setting alone) so it is not wise to leave the information out in the case of the user using a wrong thing altogether...

Again, this fine difference will be lost on people. This is better to be communicated by examples in the manual that people can copy/paste.

iLPdev · 2020-09-13T23:58:45Z

With this PR, discMapper cleanly set the map font to the gameicons.net ttf following installation by sysInstallPackage.

Thanks for this, Stephen.

SlySven · 2020-09-24T02:49:42Z

Sorry, have taken my eye off this one whilst looking at 64-bit Windows build infrastructure.

Looks good, can you flip the boolean so if you don't supply it, and the requested font isn't present, the nearest requested is used instead? We aren't a medical device here, so a better experience by default wins over precision.

😕 I don't think you've got a grasp of the UI logic. The boolean, if true forces the use of glyphs from that font only - so that if the font doesn't have the glyph requested the � glyph will be used, whereas if if is false then the best alternative from all the fonts in the system will be used. This matches the checkbox option "Only use glyph from this font" on the preferences... and the default for a new profile is for a false (which corresponds to the unchecked) option. It also means that - unlike the preferences dialogue that uses a QFontComboBox and thus is constrained to only allow selection of a font that is present it would be possible to get a non-existent font AND then providing a true boolean argument is just going to show �s all over the place. Not sure if there is a way around that... 😟

The reasons for forcing the use of a single font is for if it is a special font with perhaps glyphs being used from a PUA which, by definition (although the ConScript Unicode Registry and others do try to coordinate usages) are not standardised and which cannot thus be found in other fonts. {As it happens I have done this in the distant past in order to generate some box drawing characters with exits and doors for a custom TinTin++ ASCII 3x6 Map display - which I made nicely interactive when saved as HTML and loaded into a web-browser!} :

vadi2 · 2020-09-24T17:55:43Z

@SlySven yes, I was wrong about. The boolean is OK then - if you don't supply it, it'll use an approximation from another font.

SlySven · 2020-09-26T23:00:28Z

@ SlySven yes, I was wrong about. The boolean is OK then - if you don't supply it, it'll use an approximation from another font.

Sort of yes: if you don't supply it (or use a nil) it won't be changed, the default false setting will use the best alternative from another font, but if it is set to true it will only use the specified font (which had better be there...!)

I'll try and get around to documenting this in the wiki and cut out the detailed error message/rename the function... watch this space.

vadi2 · 2020-09-27T03:33:55Z

That's confusing... nil should be same as false.

SlySven · 2020-09-28T18:41:25Z

No - nil is a placeholder, meaning "no change to current setting" (which is the same as not providing that argument in the first place) - false and true actually reset or set the setting (respectively). Maybe it will be clearer when I write the Wiki entry.

This is so that one can change the second or third setting without knowing about the first or second. Say I needed to tweak the stretch factor from 1.1 to 1.2 - I can then do that by calling:

    setupMapSymbolFont(nil, nil, 1.2)

vadi2 · 2020-09-29T07:14:29Z

@keneanung @demonnic what do you reckon?

demonnic · 2020-09-29T15:25:37Z

This is a pattern we have a couple places (Geyser.Label:echo() springs to mind) but I'm not sure that I really like it when the parameter in question is a boolean, as that's one place nil is often used interchangably with false in Lua. And there are reasons why there are separate functions to wrap the (nil, thing, nil) cases for Geyser.Label:echo(), trying to support nondevelopers with that pattern is more difficult.

And if we're going to have to write wrapper functions around those cases to make it easier for people, then why not just write it as three functions to start?

SlySven · 2020-09-30T19:39:53Z

And if we're going to have to write wrapper functions around those cases to make it easier for people, then why not just write it as three functions to start?

That would mean two more functions in the TLuaInterpreter class ... 🙁 (the current proposal does sort of reflect why I used setup rather than set in the function name as it provides a single function that can adjust one, two or all three configurables collectively or independently!) - though that has just prompted me to consider - dues the Lua argument handling trimming off excess nils at the end of an argument list - I thought it did. So that: setupMapSymbolFont("Some Font", nil, nil) will silently discard the two nils so that the function will effectively see: setupMapSymbolFont("Some Font"), right?

vadi2 · 2020-10-01T05:58:25Z

The C api can distinguish between the two, but it would be a bad practice to actually do it

SlySven · 2020-10-05T03:41:33Z

.... that's one place nil is often used interchangably with false in Lua. ...

In this situation, however, I think we do need what might be considered a tri-state for this option and a boolean makes sense to me...

SlySven · 2021-03-19T17:57:40Z

Reverting to draft as although conflicts have just been resolved the new TLuaInterpreter functions in this PR need updating to replace some boilerplate code with some new internal validation functions...

github-actions

clang-tidy made some suggestions

github-actions · 2021-03-19T18:04:19Z

src/TLuaInterpreter.cpp

+    if (!(lua_isnil(L, ++s) || lua_isstring(L, s))) {
+        lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (symbol font name as string is optional {use nil for no change, empty string for default}, got %s!)", s, luaL_typename(L, s));
+        return lua_error(L);
+    } else if (lua_isstring(L, s)) {


warning: do not use else after return [readability-else-after-return]

} else if (lua_isstring(L, s)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

github-actions · 2021-03-19T18:04:19Z

src/TLuaInterpreter.cpp

+        if (!(lua_isnil(L, ++s) || lua_isboolean(L, s))) {
+            lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (only use specified font as boolean is optional, got %s!)", s, luaL_typename(L, s));
+            return lua_error(L);
+        } else if (lua_isboolean(L, s)) {


warning: do not use else after return [readability-else-after-return]

} else if (lua_isboolean(L, s)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

github-actions · 2021-03-19T18:04:19Z

src/TLuaInterpreter.cpp

+        fontFactorPresent = true;
+        // Round the given factor to 2 decimal places to match the resolution of
+        // the preferences' setting:
+        fontFactorRequired = qRound(100 * lua_tonumber(L, s)) / 100.0;


warning: narrowing conversion from double to float [bugprone-narrowing-conversions]

fontFactorRequired = qRound(100 * lua_tonumber(L, s)) / 100.0; ^

github-actions · 2021-03-19T18:04:19Z

src/TLuaInterpreter.cpp

+        fontFactorPresent = true;
+        // Round the given factor to 2 decimal places to match the resolution of
+        // the preferences' setting:
+        fontFactorRequired = qRound(100 * lua_tonumber(L, s)) / 100.0;


warning: narrowing conversion from double to float [cppcoreguidelines-narrowing-conversions]

vadi2 · 2021-03-24T08:50:36Z

OK, will re-review after you get that sorted.

Kebap · 2021-06-11T11:56:58Z

Moved preliminary documentation to https://wiki.mudlet.org/w/Area_51

vadi2 · 2021-10-13T10:38:37Z

Closing as it's a stale PR, please re-open when ready 👍

SlySven added enhancement low labels Aug 22, 2020

SlySven requested a review from a team as a code owner August 22, 2020 21:51

SlySven requested review from a team August 22, 2020 21:51

iLPdev mentioned this pull request Aug 26, 2020

Set mapper font on install iLPdev/discMapper#115

Open

vadi2 reviewed Sep 13, 2020

View reviewed changes

vadi2 assigned SlySven Sep 13, 2020

Merge branch 'development' into Enhance_addLuaApiforMapFont

818e603

SlySven marked this pull request as draft March 19, 2021 17:56

github-actions bot reviewed Mar 19, 2021

View reviewed changes

vadi2 assigned vadi2 and unassigned SlySven and vadi2 Mar 24, 2021

vadi2 assigned SlySven Mar 24, 2021

vadi2 closed this Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance: add Lua API for map symbol font #4038

Enhance: add Lua API for map symbol font #4038

SlySven commented Aug 22, 2020 •

edited

Loading

add-deployment-links bot commented Aug 22, 2020 •

edited

Loading

vadi2 commented Aug 22, 2020

tdk1069 commented Aug 23, 2020

vadi2 left a comment

vadi2 Sep 13, 2020

SlySven Sep 24, 2020

vadi2 Sep 24, 2020

vadi2 Sep 13, 2020

SlySven Sep 24, 2020

vadi2 Sep 24, 2020

iLPdev commented Sep 13, 2020

SlySven commented Sep 24, 2020

vadi2 commented Sep 24, 2020

SlySven commented Sep 26, 2020

vadi2 commented Sep 27, 2020

SlySven commented Sep 28, 2020 •

edited

Loading

vadi2 commented Sep 29, 2020

demonnic commented Sep 29, 2020

SlySven commented Sep 30, 2020

vadi2 commented Oct 1, 2020

SlySven commented Oct 5, 2020

SlySven commented Mar 19, 2021

github-actions bot left a comment

github-actions bot Mar 19, 2021

github-actions bot Mar 19, 2021

github-actions bot Mar 19, 2021

github-actions bot Mar 19, 2021

vadi2 commented Mar 24, 2021

Kebap commented Jun 11, 2021

vadi2 commented Oct 13, 2021

	int TLuaInterpreter::setupMapSymbolFont(lua_State* L)
	int TLuaInterpreter::setMapSymbolFont(lua_State* L)

	lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (symbol font name as string is optional {use nil for no change, empty string for default}, got %s!)", s, luaL_typename(L, s));
	lua_pushfstring(L, "setupMapSymbolFont: bad argument #%d type (symbol font name as string is optional, got %s!)", s, luaL_typename(L, s));

Enhance: add Lua API for map symbol font #4038

Enhance: add Lua API for map symbol font #4038

Conversation

SlySven commented Aug 22, 2020 • edited Loading

add-deployment-links bot commented Aug 22, 2020 • edited Loading

Test versions

vadi2 commented Aug 22, 2020

tdk1069 commented Aug 23, 2020

vadi2 left a comment

Choose a reason for hiding this comment

vadi2 Sep 13, 2020

Choose a reason for hiding this comment

SlySven Sep 24, 2020

Choose a reason for hiding this comment

vadi2 Sep 24, 2020

Choose a reason for hiding this comment

vadi2 Sep 13, 2020

Choose a reason for hiding this comment

SlySven Sep 24, 2020

Choose a reason for hiding this comment

vadi2 Sep 24, 2020

Choose a reason for hiding this comment

iLPdev commented Sep 13, 2020

SlySven commented Sep 24, 2020

vadi2 commented Sep 24, 2020

SlySven commented Sep 26, 2020

vadi2 commented Sep 27, 2020

SlySven commented Sep 28, 2020 • edited Loading

vadi2 commented Sep 29, 2020

demonnic commented Sep 29, 2020

SlySven commented Sep 30, 2020

vadi2 commented Oct 1, 2020

SlySven commented Oct 5, 2020

SlySven commented Mar 19, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Mar 19, 2021

Choose a reason for hiding this comment

github-actions bot Mar 19, 2021

Choose a reason for hiding this comment

github-actions bot Mar 19, 2021

Choose a reason for hiding this comment

github-actions bot Mar 19, 2021

Choose a reason for hiding this comment

vadi2 commented Mar 24, 2021

Kebap commented Jun 11, 2021

vadi2 commented Oct 13, 2021

SlySven commented Aug 22, 2020 •

edited

Loading

add-deployment-links bot commented Aug 22, 2020 •

edited

Loading

SlySven commented Sep 28, 2020 •

edited

Loading