Support soft wrap for line numbers plugin

[VitaliyR]

Hello.

In my past project I had a requirement to highlight code, show line numbers and have support of soft wrap lines (so no horizontal scroll).

I found that originally line numbers plugin is very simple and doing its job very well but when it comes to white-space: pre-wrap - it is breaks.

I made a fix which adds support of soft wrap for this plugin.

Let's discuss it?

Generally the most interesting part, probably, is in 'sizer' functionality. It is calculating line-height. Why I not used line-height property? Because there are different problems with it: it can be line-height: normal (so God knows how much is that), the browser zoom level can be changed (and line-height actually is inc/decreasing) etc.

So, maybe it is a bit silly, but still working solution :) (any ideas how to make it better? Would be nice to discuss)

[Golmote]

Why the fallback? In which case would we get there?

[VitaliyR]

If you are about line 21-23:

Empty line? Something like:

var a = 5;

var b = 6;

[VitaliyR]

Oh, probably it was legacy from old scenario, and can be deleted.
in forEach block empty line height calculating with same scenario.

I can update the pull request with removing this lines (20-23).

Everything else?

[VitaliyR]

Any updates on this one?

[Golmote]

This looks quite good to me, though I did not take the time to properly download it to test it.

Did you make sure it was still compatible with other plugins, different themes, etc.?

[VitaliyR]

@Golmote one more update - I've updated test.html with support for testing plugins. I've tested in different browsers - everything is fine for me :)

[LeaVerou]

Thanks for contributing!
However, it’s very difficult for us to review it and choose what to merge when you send one big pull request for multiple changes. Please split the changes into one PR for the test drive page and one for the soft breaks? Thanks!

[LeaVerou]

On the topic of separate pull requests, if you wish to modify the general code structure of a file, please send a separate PR to do that and a separate PR for any other changes. In general, the smaller the PRs (obviously as long as they’re coherent), the better. From a quick look, it seems that the changes in the line numbers plugin are way more extensive than required for this change (but I might be wrong).

[LeaVerou]

Also, I’m not a huge fan of using a line-numbers-break-word class here. Why not just get the computed style of white-space and if it’s pre-wrap or normal do the thing required to have proper line numbers with wrapped code? This would basically always do what the user expects, transparently, without having them think about what they need.

[VitaliyR]

@LeaVerou, definitely, I will separate them soon when I will have time for it.

About class name. It was just my decision. I can make it to be automatically, if you wish so.

[VitaliyR]

Okay, so I've separated updates to test.html to separate branch/pr at #592.

Also I have updated current one, so now it is determining automatically soft-wrap as @LeaVerou proposed.

@LeaVerou, @Golmote, what can you say about this?

[Golmote]

I can say that the indentation is a bit messed up right now. Please use tabs (and this applies to CSS too).
Also, please don't include the .gitignore file in this PR. (I just added .DS_Store to the list myself (1707e4e))

Now about the plugin code itself:

• The way you add the resize listener makes it impossible (or at least hard) to apply the plugin dynamically on code blocks, which should normally be doable by calling highlightElement() (which will execute the hook). I guess you could maintain a list of code blocks that require the resize check to allow this. (I'm not sure I'm making my point here, so feel free to tell me if you don't get what I mean)
• The above point would also solve the issue of getting the computed style in each call of _resizeElement() since it would be called only for code blocks that have the soft-wrap.
• The _resizeElement() function should probably be kept as small as possible since it's called on the resize event. For example, you could create lineNumberSizer element in the hook.

[VitaliyR]

@Golmote the previous implementation was just about it - just when you adding a class - everything is rock & roll.
We can also move computed style to afterHighlight hook, so it would check if the block has proper white-space - and if it is - just add a class to it. (and, also, we can move back lineNumberSizer to afterHighlight hook). So, in the _resizeElement we would just query for .line-numbers.break-word (or whatever class name) and thats it.

What do you think?

P.S. Yes, already removed that gitignore from my branch, sorry for that.

[nuba]

+1! :)

[Golmote]

Sorry for the long time waiting. It appears to work just fine. Thank you very much for this contribution!

[VitaliyR]

@Golmote wow, man, 2.5 years. Amazing, I'm already married and visited 8 countries by this time :D

[Golmote]

@VitaliyR Better late than never!

[siavashs]

I seems line highlighting does not work properly when soft wrap is enabled.

[Golmote]

@siavashs Indeed I should have expected that. Those are two different plugins... I'll open a new issue so that it can be handled at some point.