Skip to content

FlyoutLabels in UserWindows#3394

Closed
Edru2 wants to merge 3 commits intoMudlet:developmentfrom
Edru2:FlyoutLabels-in-UserWindows
Closed

FlyoutLabels in UserWindows#3394
Edru2 wants to merge 3 commits intoMudlet:developmentfrom
Edru2:FlyoutLabels-in-UserWindows

Conversation

@Edru2
Copy link
Copy Markdown
Member

@Edru2 Edru2 commented Mar 9, 2020

Brief overview of PR changes/additions

This change allows flyout labels to be put into UserWindows.

With this change it is also possible to put flyoutlabels in nested containers.

wiki documentation that flyout labels cannot be put into containers is not valid anymore.

Motivation for adding to Mudlet

It wasn't possible to add flyout labels to UserWindows or containers with more then one level of depth

Other info (issues closed, discussion etc)

This change allows flyout labels to get into UserWindows.

With this change it is also possible to put flyoutlabels in nested containers.

wiki documentation that flyout labels cannot be put into containers is not valid anymore.
@Edru2 Edru2 requested a review from a team March 9, 2020 13:09
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Mar 9, 2020

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.

end
-- search down one or more levels to enable nesting in a container
if self:getWindow(label, v) then
return self:getWindow(label, v)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to make this infinitely recursive by accident, by putting a child into the grandparent or somesuch?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think so.
It should be exactly the same recursive function as used in show().
The only way (in my opinion) it could be infinitely recursive would be if a container is in it's own container what shouldn't be possible.

Btw I'm thinking about a rewrite of some FlyoutLabel code but I'm not sure I will do that yet.
In my opinion there should be an easier way of doing this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A rewrite sounds good, this code design is a bit messy.

Copy link
Copy Markdown
Member Author

@Edru2 Edru2 Mar 11, 2020

Choose a reason for hiding this comment

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

Now I got totally rid of that part.
Was simpler then i thought but retesting is needed.

Edru2 added 2 commits March 11, 2020 17:49
Get's rid of the getWindow function
and also the closelevels function is less messy
Copy link
Copy Markdown

@ndsamuelson ndsamuelson left a comment

Choose a reason for hiding this comment

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

I see no issues, and it seems the CI build has fixed itself.

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.

Looks good! 👍

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 13, 2020

Thanks for reviewing @ndsamuelson! Feel free to review more PRs :)

Closing this as #3426 has been merged and this PR contains the code from there.

@vadi2 vadi2 closed this Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants