<table><tr><td style="">michalsrb added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12662">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D12662#257342" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D12662#257342</a>, <a href="https://phabricator.kde.org/p/brauch/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@brauch</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Looks good from the implementation too so far. One thing I do not see is any changes to the cursorToX / xToCursor functions, is there really no change required there?</p></div>
</blockquote>
<p>No change was really needed. As the spaces are created when laying out the line, either by offsetting the start of the line or by formatting the text, it gets stored in the layout and the cursorToX / xToCursor work as expected. Even moving the cursor around with arrow keys works nicely (the cursor moves to the closest position above/below even if one of the lines is shifted).</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Some things which come to my mind for testing would be:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">is selection rendered correctly if it includes notes, at the end, beginning, or middle of lines, also mult-line selections?</li>
</ul></blockquote>
<p>I thought regular selection works fine, but actually I just found a glitch; if there is a note at the beginning of the line, it does not render the selection background under it:</p>
<p><a href="https://phabricator.kde.org/F5831112" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F5831112: Screenshot_20180502_205152.png</a></p>
<p>A selection in block mode behaves like if the notes are not there:</p>
<p><a href="https://phabricator.kde.org/F5831117" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F5831117: Screenshot_20180502_205212.png</a></p>
<p>I can't tell whether it is good or bad behavior. Do you think it should stay this way - selecting the block of the real text, or should it instead select a "visual" block?</p>
<p>I was thinking it would be best to avoid this problem and hide the notes when doing block mode selection, but should that be done by ktexteditor directly, or by the user of the interface? I also imagine that anything that uses it would provide some quick on/off toggle for cases like this.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">what happens when clicking or dragging from or into the notes?</li>
</ul></blockquote>
<p>Clicking into the note always places the text cursor on the right side of the note - in other words, the note acts as extension of the letter on its left. Dragging makes normal selection from that position.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">does it still work properly with dynamic word-wrap on?</li>
</ul></blockquote>
<p>Hmm, looks like notes at the beginning of the text ruin the breaking of the line:</p>
<p><a href="https://phabricator.kde.org/F5831141" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F5831141: Screenshot_20180502_212929.png</a></p>
<p>And it also does not place the notes well if the wrapped line keeps keeps indentation, like in this case:</p>
<p><a href="https://phabricator.kde.org/F5831123" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F5831123: Screenshot_20180502_210236.png</a></p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">does it work properly with code-folding? what happens if a note is at the border of a folding region?</li>
</ul></blockquote>
<p>It works fine. When a line is hidden, so are the notes in it.</p>
<p>One more thing that needs to be decided: At this moment if a note is placed past the end of the text too far to the right and word-wrap is on, it does not wrap, but renders partially or completely out of view. (This happens kinda by default as the layout of the line is not modified for notes out of the text.) My thinking was that it does not matter as one would either place notes in the text, or outside at a column that will be visible without scrolling/wrapping. But if you think it should wrap, I can try to find a way to make that happen.</p>
<p>Thank you for the review, I'll fix these problems and make the changes you noted.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R39 KTextEditor</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12662">https://phabricator.kde.org/D12662</a></div></div><br /><div><strong>To: </strong>michalsrb, KTextEditor<br /><strong>Cc: </strong>brauch, Frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann<br /></div>