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

[NETBEANS-977] Improve text layout in word wrap mode. #598

Merged
merged 1 commit into from Sep 26, 2018

Conversation

eirikbakke
Copy link
Contributor

Improve text layout for the NetBeans editor's "word wrap" feature. This makes the word wrap behavior more in line with that of other editors, and makes wrapped text more readable. See detailed description and before/after screenshot (also attached here) at https://issues.apache.org/jira/browse/NETBEANS-977 .

wrappingdiff

@emilianbold
Copy link
Member

emilianbold commented Jun 22, 2018

The patch is clean and has tests but it would take me some time to understand what you did there as I'm not familiar with the area. It would be nice to see Miloslav Metelka show up here.

I see you show the line wrap character only if isNonPrintableCharactersVisible is true but honestly I don't know which GUI setting toggles this.

@eirikbakke
Copy link
Contributor Author

The isNonPrintableCharactersVisible setting can be triggered via the "Toggle Non-printable Characters" action--I assigned it a keyboard shortcut in the Preferences pane to be able to invoke it.

Yeah, when I looked at the historical "git blame", Miroslav was the one who wrote all the related logic (most of it back in 2010).

@eirikbakke
Copy link
Contributor Author

I have added a few more known issues related to word wrapping in the NetBeans editor, in case anyone who knows the code strolls through and wants to comment:

https://issues.apache.org/jira/browse/NETBEANS-978
https://issues.apache.org/jira/browse/NETBEANS-979
https://issues.apache.org/jira/browse/NETBEANS-980

(Fixing these few word wrapping bugs would increase the IDE's value as a general-purpose editor rather than just a code editor, at least for people who like to write long paragraphs of text without explicit line breaks. Though my own motivation for getting these fixed is because my spreadsheet-like NetBeans Platform application uses the NetBeans EditorKit for its table cell editor and formula bar.)

@emilianbold
Copy link
Member

I've sent an email to ask about Miloslav, let's hope he'll show up soon.

@eirikbakke
Copy link
Contributor Author

With regards to the problems with the Arrow Up/Arrow Down/Home/End actions in wrapped paragraphs, I've done some investigations that I typed up in https://issues.apache.org/jira/browse/NETBEANS-980 (also closing NETBEANS-979 as a duplicate of the latter, since they turned out to be variations on the same problem).

But this pull request can be reviewed and applied independently of those other issues.

@geertjanw
Copy link
Member

@eirikbakke, how confident are you about this patch, you're currently the most knowledgeable in this area and we should try to not necessarily rely on Oracle staff anymore as far as possible. If there are specific areas where you need help or are less confident about, please say so, but on the face of it this looks like an awesome enhancement that we should accept.

@jlahoda
Copy link
Contributor

jlahoda commented Jun 27, 2018 via email

@geertjanw
Copy link
Member

Would be good for at least @sdedic to comment, and let's merge then.

@eirikbakke
Copy link
Contributor Author

There is one issue that I see now--since there is less margin on the right now, putting the cursor at the last character on a wrap line makes the editor scroll horizontally by a few pixels to put it fully into view, even if the last character is just a whitespace. I should probably fix that before the patch is applied.

Now busy with another deadline; will revisit in a couple of weeks.

@mmetelka
Copy link

mmetelka commented Jul 3, 2018

The patch seems ok to me, I just do not have all the details regarding the line wrapping code cached in my memory right now.
Visibility of a speacial char denoting line continuation depends on a personal preference and I do not have a strong opinion regarding this. We can imho have it as proposed i.e. only show it when View->Show Non-printable Characters is turned on.

@geertjanw
Copy link
Member

OK, @eirikbakke, its been reviewed in different ways now, even by @mmetelka, just say the word when you're done -- and/or if more reviews are needed after you make further tweaks -- and then let's merge this cool enhancement.

@eirikbakke
Copy link
Contributor Author

Great! I've actually prepared a few more commits that should be reviewed as part of this patch (improving horizontal scroll behavior in view of the new wrapping policy). But before I push them for review, I'd like to make sure I'm not breaking any existing unit tests.

I'm having trouble [1] running unit tests for the editor.lib2 module, even before the patch is applied. Is anyone able to run the test suite successfully, in particular for this module?

[1] "task failed due to: java.lang.NoClassDefFoundError: org/netbeans/editor/GuardedDocument", e.g. in org.netbeans.modules.editor.lib2.highlighting.HighlightingManagerTest. See email thread on the dev list with subject "Does Travis and/or Jenkins run the NetBeans test suite?"

@eirikbakke
Copy link
Contributor Author

I've added a few more commits to this pull request, to improve the editor's horizontal scrolling behavior to work well with line wrapping enabled.

I've also confirmed that there are no new broken tests in the editor, editor.lib, and editor.lib2 modules as a result of these commits. Though note that both the editor.lib2 module and the editor module had broken tests before this patch was applied. And the latter modules' tests don't seem to be run by Travis nor Jenkins. (See the email thread on the dev list with the subject "Does Travis and/or Jenkins run the NetBeans test suite?")

@geertjanw
Copy link
Member

@eirikbakke, how far along is this from your point of view? Do you need a review, of what specifically, is it ready to be merged from your point of view, and how confident are you about it?

@eirikbakke
Copy link
Contributor Author

I think this patch as well as #603 is ready now. I have not been able to find any new bugs or test failures introduced by them.

It might be a good general policy to always require at least a cursory review of new commits by someone else than the original author. But there are no blockers from my point of view.

@eirikbakke
Copy link
Contributor Author

The upstream repo changed its directory structure, which means the pull request got a merge conflict. Now trying to figure out how to merge or rebase it from my side...

@eirikbakke eirikbakke force-pushed the NETBEANS-977 branch 2 times, most recently from a38784d to 7364449 Compare September 9, 2018 02:23
@eirikbakke
Copy link
Contributor Author

I rebased (and force-pushed) this pull request since changes in the upstream directory structure caused a merge conflict.

I've had the patch enabled (together with #603 ) in both my development IDE and my NetBeans Platform application for about a week now, with no ill effects, so I think both of these pull requests are ready to be merged.

final TextLayout lineContTextLayout = getLineContinuationCharTextLayout();
final float lineContTextLayoutAdvance =
lineContTextLayout == null ? 0f : lineContTextLayout.getAdvance();
availableWidth = Math.max(getVisibleRect().width, 4 * getDefaultCharWidth() + lineContTextLayoutAdvance);
Copy link
Member

Choose a reason for hiding this comment

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

original code did not change availableWidth if lineContTextLayout == null; is it OK to alter it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; the getLineWrapType() != LineWrapType.NONE check is sufficient. Previously, getLineContinuationCharTextLayout would never return null in practice--only if the font could not display the line continuation character or its alternate. And if that ever happened, there would have been an NPE when WrapInfo calls paintTextLayout with the null layout.

I changed this so that getLineContinuationCharTextLayout can now return null without error (and added Javadoc to that effect). Rather than disabling line wrapping altogether, this simply avoids displaying displaying the line continuation character. Then I made this the default, unless the user opts to display hidden characters (paragraph marks etc).

I just looked over the call sites of getLineContinuationCharTextLayout again; they seem to do the right thing.

? preferredMaximumBreakOffset
: bi.preceding(preferredMaximumBreakOffset);
if (ret == BreakIterator.DONE)
ret = preferredMaximumBreakOffset;
Copy link
Member

Choose a reason for hiding this comment

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

Use {} around if-ed statement; maybe return preferredMaximumBreakOffset immediately for better clarity (case preferredMaximumBreakOffset == 0 handled at 801).

If bi.preceding(...) == 0, that is boundary at char 0, is it OK to search forwards (as seen below) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed cosmetic issues.

Yes, that case is intentional. When the break line is in the middle of a word, we normally scan backwards to try to find the beginning of that word, and then we break there. But if the beginning of the word is in fact at the beginning of the current break line, then that means we have a super-long word that spans the entire viewport, and we must "give up" and break after the word instead, beyond the preferred break offset (requiring horizontal scrolling in the editor). This case is exercised many times when AdjustBreakOffsetToWordTest is run (I just checked to be sure).

This squashed commit combines the following commits (1-5 were seen in the original pull request, before rebase to fix merge conflict due to directory structure change):
1) Improve text layout in word wrap mode.
2) Refactor EditorCaret to avoid repeated code for getting the JViewport.
3) Fix scrollRectToVisible behavior for the improved text wrap layout.
4) Modify the word wrapping policy slightly to allow at most one whitespace character to trail the preferred wrap width. This avoids having to paint the caret outside the wrapped editor viewport.
5) Improve text caret behavior when typing at the end of a wrapped paragraph.
6) (2018-09-13:) Cosmetic adjustments after pull request comments. Use {} around one-line ifs. Also do "return preferredMaximumBreakOffset" instead of "ret = preferredMaximumBreakOffset" in adjustBreakOffsetToWord, as suggested in PR comment. They are logically equivalent at this point in the code.
@eirikbakke
Copy link
Contributor Author

I incorporated sdedic's comment and pushed again. (After having to rebase and force-push to fix the directory structure merge conflict, it seems I have to force push again to get the pull request updated. Sorry--I hope it doesn't make the PR history too confusing.)

@geertjanw
Copy link
Member

Is this ready to be merged? Would be great to have in Apache NetBeans 10.

@eirikbakke
Copy link
Contributor Author

Should be ready, yes.

@geertjanw
Copy link
Member

OK, merging, this has been reviewed a lot and everyone is happy.

@eirikbakke eirikbakke deleted the NETBEANS-977 branch October 5, 2022 13:26
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.

None yet

6 participants