Don't allow setting a 0 width font by vadi2 · Pull Request #2865 · Mudlet/Mudlet

vadi2 · 2019-07-26T03:20:40Z

Brief overview of PR changes/additions

Don't allow setting a 0 width font.

Motivation for adding to Mudlet

We need to divide by the font width in a few places and division by zero is a no-no. Also, such fonts don't seem to be practical fonts you could use in gaming!

Other info (issues closed, discussion etc)

I know we shouldn't be measuring W and instead using averageCharWidth() or similar function - but improvement that is outside of the scope of this fix.

Fix #2590, fix #2391.

# Conflicts: # src/Host.h

add-deployment-links · 2019-07-26T03:20:46Z

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 · 2019-07-26T03:21:12Z

@Eraene @Xekon please test, thanks!

SlySven · 2019-07-28T00:10:06Z

In revising a flaw in your #2869 I am also switching to use the QFontMetrics::averageCharWidth() - because the code concerns establishing the position of the mouse cursor in a line of TConsole text - which will interact with this PR...

vadi2 · 2019-07-28T05:18:34Z

OK - but is this PR good to go? Would rather not have a merge conflict.

keneanung

Looks good in general, but I'm not quite happy with a few details (which might result in greater work).

src/Host.cpp

src/TTextEdit.cpp

keneanung · 2019-07-29T07:18:03Z

src/TLuaInterpreter.cpp

-            pHost->mDisplayFont = displayFont;
+            if (!pHost->setDisplayFont(font)) {
+                lua_pushnil(L);
+                lua_pushstring(L, "specified font is invalid (average character width is 0)");


This works for now, but show knowledge about the internal workings of setDisplayFont(). Can we return an error string from the function instead as well?

There is no error string from the function though?

That's right. This is why I'd move this message into setDisplayFont instead.

I see what you mean.

Added, have a look - but I'm not sure I like it. The other choice is to just return an std::optional which we now have access to, but that also doesn't seem ideal when you actually try using it...

src/XMLimport.cpp

keneanung · 2019-07-29T07:20:49Z

src/dlgProfilePreferences.cpp

    font.setPointSize(mFontSize);
-    if (pHost->mDisplayFont != font) {
-        pHost->mDisplayFont = font;
+    if (pHost->getDisplayFont() != font && pHost->setDisplayFont(font)) {


If the user selects such a 0 width font now, nothing happens when they click save. No message or error. They will wonder why.

SlySven · 2019-07-29T18:27:32Z

Is it possible that (after my proposed revisions to) #2869 we will be using QFontMetrics::averageCharWidth() instead and we won't get the same crashes that we have been from selecting (as a known test case) the esint10 font?

vadi2 · 2019-07-29T19:25:15Z

Not sure, that function isn't guaranteed not to return 0, is it...

…th-fonts # Conflicts: # src/TLuaInterpreter.cpp # src/XMLimport.cpp # src/dlgProfilePreferences.cpp

keneanung · 2019-08-06T13:41:45Z

src/dlgProfilePreferences.cpp

    QFont font = fontComboBox->currentFont();
    font.setPointSize(mFontSize);
-    if (pHost->getDisplayFont() != font && pHost->setDisplayFont(font)) {
+    if (auto [setNewFont, errorMessage] = pHost->setDisplayFont(font); pHost->getDisplayFont() != font && setNewFont) {


This makes the first condition obsolete because you set the font before you check if it is different. Also, why don't you use the error message here? My point from earlier still stands: Mudlet will silently do nothing.

Just haven't gotten around to showing the message yet, sorry!

keneanung · 2019-08-06T13:44:38Z

src/Host.cpp

    const QFontMetrics metrics(font);
    if (metrics.averageCharWidth() == 0) {
-        return false;
+        return std::make_pair(false, QStringLiteral("specified font is invalid (its letters have 0 width)"));


Hm, this is unfortunate. On the one hand, we may want to translate this when showing on the GUI, but we don't want to translate this when using it in Lua code...

To be honest showing "the selected font has 0 width characters" is going to be nonsense to the majority of the people. We can just say "this isn't a valid font" in the UI and they'll ignore it or ask for help for it.

You're probably right.

# Conflicts: # src/dlgMapper.cpp # src/dlgProfilePreferences.cpp

# Conflicts: # src/TConsole.cpp # src/TLuaInterpreter.cpp # src/TTextEdit.cpp

demonnic

lgtm

Requested changes addressed

* WIP * Fix 0 width fonts crashing Mudlet * Review feedback * Return status + error msg * Fix merge error * Show error msg if invalid font is picked * Reset tab index back to the first * Strip PlaceholderText NoBrush - still doesn't work with Qt 5.11

vadi2 added 3 commits July 21, 2019 23:03

WIP

d6149d0

Merge remote-tracking branch 'origin/development' into fix-0-width-fonts

702238c

# Conflicts: # src/Host.h

Fix 0 width fonts crashing Mudlet

006b2a1

vadi2 added this to the 4.0.0 milestone Jul 26, 2019

vadi2 requested a review from a team as a code owner July 26, 2019 03:20

vadi2 requested review from a team July 26, 2019 03:20

keneanung previously requested changes Jul 29, 2019

View reviewed changes

vadi2 modified the milestones: 4.0.0, 4.1.0 Jul 30, 2019

vadi2 self-assigned this Aug 2, 2019

vadi2 added 3 commits August 6, 2019 08:14

Merge remote-tracking branch 'origin/l10n_development' into fix-0-wid…

fc44238

…th-fonts # Conflicts: # src/TLuaInterpreter.cpp # src/XMLimport.cpp # src/dlgProfilePreferences.cpp

Review feedback

05a79b3

Return status + error msg

766acc5

keneanung reviewed Aug 6, 2019

View reviewed changes

vadi2 added 5 commits August 15, 2019 18:56

Merge remote-tracking branch 'origin/development' into fix-0-width-fonts

02475c0

# Conflicts: # src/dlgMapper.cpp # src/dlgProfilePreferences.cpp

Merge remote-tracking branch 'origin/development' into fix-0-width-fonts

819f03b

# Conflicts: # src/TConsole.cpp # src/TLuaInterpreter.cpp # src/TTextEdit.cpp

Fix merge error

2ccc1bd

Show error msg if invalid font is picked

257ffcf

Reset tab index back to the first

a2912be

vadi2 assigned demonnic and SlySven and unassigned vadi2 Aug 22, 2019

Strip PlaceholderText NoBrush - still doesn't work with Qt 5.11

686e44b

demonnic approved these changes Aug 22, 2019

View reviewed changes

vadi2 merged commit 66a4d34 into Mudlet:development Aug 22, 2019

vadi2 deleted the fix-0-width-fonts branch August 22, 2019 14:25

vadi2 mentioned this pull request Aug 22, 2019

Fix up font loading issues #3015

Merged

Kebap mentioned this pull request Jan 19, 2021

show error upon font display #4658

Closed

Uh oh!

Conversation

vadi2 commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

vadi2 commented Jul 26, 2019

Uh oh!

SlySven commented Jul 28, 2019

Uh oh!

vadi2 commented Jul 28, 2019

Uh oh!

keneanung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven commented Jul 29, 2019

Uh oh!

vadi2 commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demonnic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vadi2 commented Jul 26, 2019 •

edited

Loading

add-deployment-links bot commented Jul 26, 2019 •

edited

Loading