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

Add an auto wrap function to the mini console #1708

Merged
merged 6 commits into from Jun 9, 2018

Conversation

Projects
None yet
4 participants
@oestrich
Contributor

oestrich commented May 27, 2018

Brief overview of PR changes/additions

Add an auto wrap function to the MiniConsole.

Motivation for adding to Mudlet

To let the MiniConsole auto wrap and don't worry about the size of the console.

Other info (issues closed, discussion etc)

@oestrich oestrich requested a review from Mudlet/lua-interface as a code owner May 27, 2018

@vadi2

Looks good - could you also make it so if you give wrapAt = "auto" in the constructor, it sets that as well?

@vadi2

This comment has been minimized.

Member

vadi2 commented May 27, 2018

@keneanung Geyser's :get_width() doesn't account for the scrollbars addition so it's wrong when you have a scrollbar... what do you think is the best way to handle this?

@vadi2

vadi2 approved these changes May 27, 2018

@vadi2 vadi2 added this to the 3.10.0 milestone May 27, 2018

if cons.wrapAt == "auto" then
me:setAutoWrap()
elseif cons.wrapAt then
me:setWrap(cons.wrapAt)

This comment has been minimized.

@vadi2

vadi2 May 27, 2018

Member

Tiny detail - but if I create a miniconsole with autowrap on, then decide against it, change to a number and save, self.autoWrap flag will still be on

edit: nevermind, it wouldn't be, this isn't a Mudlet-side setting. The Lua object would be recreated anew.

@@ -12,6 +12,14 @@ Geyser.MiniConsole = Geyser.Window:new({
name = "MiniConsoleClass",
wrapAt = 300, })
--- Override reposition to reset autowrap
function Geyser.MiniConsole:reposition()
Geyser.Window.reposition(self)

This comment has been minimized.

@oestrich

oestrich May 27, 2018

Contributor

Would this be better as self.parent:reposition()?

This comment has been minimized.

@keneanung

keneanung May 29, 2018

Member

I'd probably use self.parent.reposition(self). Your variant binds the parent as self.

@vadi2

This comment has been minimized.

Member

vadi2 commented May 28, 2018

FYI - I remembered calcFontSize() was fixed to have the size of the font as input, which was good when Miniconsoles were limited to the default Bistream Vera Sans Mono font. They're not anymore with setFont(), so technically calcFontSize() would lie if you set a custom font.

I have #1699 ready to address this - the only thing you would need to do is swap out the font size parameter for the miniconsole name.

function Geyser.MiniConsole:resetAutoWrap()
local fontWidth, fontHeight = calcFontSize(self.fontSize)
local consoleWidth = self.get_width()
local charactersWidth = math.floor(consoleWidth / fontWidth)

This comment has been minimized.

@SlySven

SlySven May 28, 2018

Member

👎 That is not going to work for duo-spaced (Wide CJK?) characters BTW.

@vadi2

This comment has been minimized.

Member

vadi2 commented May 28, 2018

@SlySven

This comment has been minimized.

Member

SlySven commented May 28, 2018

I don't have any quick fixes - I guess we will have to see how it works out in practice. Eventually I suspect I will have to overhaul the text painting code to actually measure the width of the text as it is displayed (either in the TConsole or a dummy surface) and wrap it when the length exceeds (the width setting × the average character width). I did try something along those lines for painting but did not get as far as revising the wrapping code but I parked it when the CJK-width PR came along - though I do still regard that as a quick and dirty fix...

@SlySven

Okay, lets suck this and see. :shipit:

--- Set the wrap based on how wide the console is
function Geyser.MiniConsole:resetAutoWrap()
local fontWidth, fontHeight = calcFontSize(self.fontSize)

This comment has been minimized.

@vadi2

vadi2 May 29, 2018

Member

If you could change this to self.name so it'll take the custom font into account (available with 1dd7427) and apply https://github.com/Mudlet/Mudlet/pull/1708/files#r191332826, it'll be good to go 💯

Tweaks for PR review
- Use `self.name` to calculate font size
- Use `self.parent.reposition`
@oestrich

This comment has been minimized.

Contributor

oestrich commented May 30, 2018

@vadi2 this should be all up to date now

@vadi2

This comment has been minimized.

Member

vadi2 commented May 30, 2018

@keneanung

I can't see anything to turn autowrap off (except creating a new console) Am I missing something?

@oestrich

This comment has been minimized.

Contributor

oestrich commented May 30, 2018

At the moment you can't. I can update the setAutoWrap function to take a boolean.

@vadi2

This comment has been minimized.

Member

vadi2 commented May 30, 2018

Let's go with enableAutowrap and disableAutowrap instead - easier for readability and autocompletion

@keneanung

The exlicit enabling/disabling is good, but it creates an issue: If I enable autowrap, but set a wrap length, it'll wrap at that position until the next reposition. This feels like a quirky behaviour.

Could you also mention in the documentation of disableAutoWrap that the programmer should use setWrap immediately after?

@vadi2

This comment has been minimized.

Member

vadi2 commented Jun 7, 2018

@keneanung good to go?

@keneanung

My first observation above is still not handled, but if you think it's neglectable, go for it.

@vadi2

This comment has been minimized.

Member

vadi2 commented Jun 9, 2018

I missed that. Let's disable manual wrap length while autowrap is on?

@keneanung

This comment has been minimized.

Member

keneanung commented Jun 9, 2018

That's probably the best way.

@vadi2

This comment has been minimized.

Member

vadi2 commented Jun 9, 2018

Okay, I've updated the PR.

@vadi2 vadi2 merged commit 0e70258 into Mudlet:development Jun 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vadi2

This comment has been minimized.

Member

vadi2 commented Jun 9, 2018

@oestrich thanks so much for this feature 🥇

@oestrich oestrich deleted the oestrich:set-auto-wrap branch Jun 29, 2018

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