Skip to content
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

fix padWithLinebreaks by switching to upstream FAIAP #1194

Merged
merged 4 commits into from Feb 26, 2019

Conversation

mrbuds
Copy link
Contributor

@mrbuds mrbuds commented Feb 21, 2019

Description

Fix adding trailing line break when editing custom code

Copy link
Contributor

@emptyrivers emptyrivers left a comment

Choose a reason for hiding this comment

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

I just want to note that this will update this library for any other addon which uses it. This may or may not have unintended side effects.

@Stanzilla
Copy link
Contributor

Did people actually complain about this?

@InfusOnWoW
Copy link
Contributor

yes people notice extra ever increasing line breaks, but this looks strange, the code explicitly checks whether the code ends in \n\n, and only adds those if they are missing so this should only ever add 2

@mrbuds
Copy link
Contributor Author

mrbuds commented Feb 22, 2019

I'll see if i can come with a fix to the function.
Current problem is :

  • it make sure the editbox always finish with 2 \n as you type
  • When you type a new block keyword all text after this keyword is indented, the 2 \n included, so last character is \n but the previous one is a space, so function add a \n at the end, which gets itself indented etc ...

@mrbuds mrbuds changed the title remove padWithLinebreaks fix padWithLinebreaks Feb 22, 2019
@mrbuds
Copy link
Contributor Author

mrbuds commented Feb 22, 2019

padWithLinebreaks restored and fixed to handle indented line breaks

@Stanzilla Stanzilla added the 🏨 Bugfix This pull request fixes an issue. label Feb 22, 2019
@Stanzilla Stanzilla added this to the 2.12.0 milestone Feb 22, 2019
if stringbyte(code, len) == linebreak then
if stringbyte(code, len - 1) == linebreak then
return code, false
local len = stringlen(code) + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I am nitpicking, but this number doesn’t really represent the length of the string anymore.

local breakfound = 0
while true do
len = len - 1
local byte = stringbyte(code, len)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might cause issues if the code is < 2 bytes long. I’m not sure how stringbyte handles inputs < 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

A simple fix would be to check if the string is extremely short, and handle that as a special case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's working as intended on an empty custom code editbox

moved byte variable out of the loop to save allocation time
@InfusOnWoW
Copy link
Contributor

So this looks like it is indeed a bug in that library and the fix is good.

But, it's a library and we overwriting everyone elses version if we ship this.

@Stanzilla how do we proceed ?

@Stanzilla
Copy link
Contributor

So I don't actually know of any other addon that uses it and there is no repo for it either, it was just passed on as a "snippet" over the years. So I'd say we can roll with it and see if anyone complains, if they do, we can change the name of the lib.

@InfusOnWoW
Copy link
Contributor

Hmm, that's not ideal, but okay let's do that.

@emptyrivers
Copy link
Contributor

emptyrivers commented Feb 23, 2019

The simple answer would be to just make this fork local. Then we can update it as needed without worrying about conflicts.

If somebody is depending on this library to be delivered publically along with WeakAuras, then they deserve a smack in the nose :)

@Stanzilla
Copy link
Contributor

We just discovered that WoWLua, Plater and Details use it so yeah

@Stanzilla
Copy link
Contributor

Maybe we should just make an official repo for it and let the other authors know

@emptyrivers
Copy link
Contributor

That’s fine with me. Do we have permission to republish like that?

@mrbuds
Copy link
Contributor Author

mrbuds commented Feb 23, 2019

Details & Plater use the same version as we do, with the fix on getCaretPos(editbox) we did
WoWLua seems to use an outdated version, their revision number is nil and i think no one saw that because WeakAuras load before WoWLua

@Stanzilla
Copy link
Contributor

Hey @grizzm0 what's the license on AllIndentsAndPurposes?

@grizzm0
Copy link

grizzm0 commented Feb 23, 2019

@Stanzilla Tagged the wrong person? ;)

@Stanzilla
Copy link
Contributor

Stanzilla commented Feb 23, 2019

@grizzm0 certainly possible, I searched for the email address on GitHub and you came up, sorry! kristofer.karlsson vs karlsson.kristofer >.<

@grizzm0
Copy link

grizzm0 commented Feb 23, 2019

@Stanzilla No worries. Always wondered who managed to steal that email from me! :o I even got an odd spelling of Kristofer. ;)

@InfusOnWoW
Copy link
Contributor

@spkrka I believe you are the original author of ForAllIndentsAndPurposes, right?

@spkrka
Copy link

spkrka commented Feb 23, 2019

Yes, that's me (but on my personal account @krka )

Here's the original repository: https://github.com/krka/ForAllIndentsAndPurposes
It appears my original repo is several revisions behind this version - perhaps I should try to reintegrate it.

Code change looks reasonable, and I can take a deeper look later. A simple thing to change could be to replace while true do with while len >= 0

(Sorry about the name steal @grizzm0 but I suspect I was in fact born before you :-) )

@krka
Copy link

krka commented Feb 24, 2019

I've made the following PR in the original repository now: krka/ForAllIndentsAndPurposes#4
It solves the same problem as this PR, but also adds some test cases for it. (And applies both for space and tab character)

@Stanzilla Stanzilla modified the milestones: 2.12.0, 2.11.6 Feb 25, 2019
@Stanzilla Stanzilla changed the title fix padWithLinebreaks fix padWithLinebreaks by switching to upstream FAIAP Feb 26, 2019
@Stanzilla Stanzilla merged commit e601498 into WeakAuras:master Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏨 Bugfix This pull request fixes an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants