Skip to content

simplify code for createLabel (suggestion)#3384

Merged
vadi2 merged 2 commits intoMudlet:developmentfrom
Edru2:simplify-createLabel
Mar 5, 2020
Merged

simplify code for createLabel (suggestion)#3384
vadi2 merged 2 commits intoMudlet:developmentfrom
Edru2:simplify-createLabel

Conversation

@Edru2
Copy link
Copy Markdown
Member

@Edru2 Edru2 commented Mar 4, 2020

Brief overview of PR changes/additions

Split the functionality of PR 3367 into two internal functions one create Label in UserWindow and the other in the MainWindow

Only functional change to PR 3367 is that fillbackground is non-optional again

Motivation for adding to Mudlet

A suggestion to simplify and make the code for createLabel easier to read

Other info (issues closed, discussion etc)

made quick test and couldn't find any issue

@Edru2
Simplified PR 3367
8b5af85 
Split the functionality of PR 3367 into to internal functions one create Label in UserWindow and the other in the MainWindow
Only functional change to PR 3367 is that fillbackground is non-optional again
@Edru2 Edru2 requested a review from a team as a code owner March 4, 2020 07:29
@Edru2 Edru2 requested review from a team March 4, 2020 07:29
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Mar 4, 2020 •
edited
Loading

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.

@Edru2 Edru2 mentioned this pull request Mar 4, 2020
Closed
vadi2
vadi2 approved these changes Mar 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Thanks! Yes this is a lot more readable. Duplicated, yes, but the price we had to pay for not duplicating was not worth it.

@Edru2
Update TLuaInterpreter.cpp
de71c34 
typo in errormsg
@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 5, 2020

@Edru2 how does this work with the setAppStyleSheet() fix you've found?

@Edru2
Copy link
Copy Markdown
Member Author

Edru2 commented Mar 5, 2020

This also fixes the issue (still there in version 4.5.2) as described in #2464
I know the fix for #2464 is related to the use of fillBackground = (lua_tointeger(L, 7) != 0);
But as this seems totally unrelated I have no Idea why this fixes this issue.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 5, 2020 •
edited
Loading

Merging this in the interests of time so it can get tested and into 4.6.

@vadi2 vadi2 merged commit 8190d12 into Mudlet:development Mar 5, 2020
vadi2 added a commit that referenced this pull request Mar 5, 2020
@vadi2
Fix createLabel (..., true) (#3384)
11d5cab 
* Simplified PR 3367

Split the functionality of PR 3367 into to internal functions one create Label in UserWindow and the other in the MainWindow
Only functional change to PR 3367 is that fillbackground is non-optional again

* Update TLuaInterpreter.cpp

typo in errormsg

(cherry picked from commit 8190d12)
atari2600tim added a commit to atari2600tim/Mudlet that referenced this pull request Mar 5, 2020
@atari2600tim
Merge pull request #3 from Mudlet/development
cc91173 
Fix createLabel (..., true) (Mudlet#3384)
@SlySven
Copy link
Copy Markdown
Member

SlySven commented Mar 7, 2020

Humm, I thought the Lua API documentation pointed out that fillBackground WAS optional but I see that it had not been so documented as such there - so changing the API is, perhaps okay.

Why does it have to be a requirement though, if it could be omitted (and was thus taken to be false as the past C++ code intended even if it did not achieve it) then the label would be drawn with the same background as whatever is underneath the label - which may or may not be what the user wanted. However aren't gauges created from three labels drawn on top of each other - so doesn't the top two need to be drawn without filling in the background - or is the Lua code {IIRC the gauge code is an external Lua script and not a C++ core feature so I do not know so much about it...} explicit about both this and the click-through option.

Of course, the clickthrough option is not mentioned at all in the Wiki documentation!.

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

Reviewers

@vadi2 vadi2 vadi2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Edru2 @vadi2 @SlySven