D29789: Make text always align with font base line

[Sven Brauch]

brauch added a comment.


  Hmm, consider though that a configuration option should be something that 
gives a choice to the user. It shouldn't be necessary to set a config option in 
order to make the program behave correctly.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D29789

To: xuetianweng, rjvbb, dhaumann, cullmann
Cc: brauch, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

[Sven Brauch]

brauch added a comment.


  In D25339#663915 , @fakefred wrote:
  
  > I second the as-an-option proposal. Hey, why not automatically increase the 
line height when CJK characters are detected?
  
  
  This issue has been around for years and actually yeah, I think that is the 
only viable solution unless somebody dives in and reworks the renderer to 
support variable line heights.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D27912: Draw inlineNotes after drawing word wrap marker

[Sven Brauch]

brauch added a comment.


  I also don't understand this. Even if the painting somehow changes, e.g. 
because some painter state is set which wasnt set before (which I do not see to 
be the case here), that should not affect the line layout, as that is computed 
separately.
  
  Very strange.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D27912

To: davidre, #ktexteditor, brauch
Cc: cullmann, brauch, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D27912: Draw inlineNotes after drawing word wrap marker

[Sven Brauch]

brauch accepted this revision.
brauch added a comment.
This revision is now accepted and ready to land.


  Looks good, thanks! Yes, it's an improvement.

REPOSITORY
  R39 KTextEditor

BRANCH
  caret

REVISION DETAIL
  https://phabricator.kde.org/D27912

To: davidre, #ktexteditor, brauch
Cc: brauch, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:7f043fbb26d4: inline notes: correctly set underMouse() for 
inline notes (authored by brauch).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26840?vs=74110=74351#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26840?vs=74110=74351

REVISION DETAIL
  https://phabricator.kde.org/D26840

AFFECTED FILES
  autotests/src/inlinenote_test.cpp
  src/view/kateview.cpp
  src/view/kateviewinternal.cpp

To: davidre, #ktexteditor, cullmann, brauch
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch added a comment.


  Ok, done ;)

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann, brauch
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch accepted this revision.
brauch added a comment.


  If you want I can integrate these changes and submit your patch, should I? 
Thanks a lot for your contribution.

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann, brauch
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch added a comment.


  I'm sorry, updateView is the wrong function to call, you need updateDirty. I 
tried it out, like this it works:
  
if (e->buttons() == Qt::NoButton) {
auto noteData = inlineNoteAt(e->globalPos());
if (noteData.m_position.isValid()) {
if (!m_activeInlineNote.m_position.isValid()) {
// no active note -- focus in
tagLine(noteData.m_position);
updateDirty();
noteData.m_underMouse = true;

noteData.m_provider->inlineNoteFocusInEvent(KTextEditor::InlineNote(noteData), 
e->globalPos());
m_activeInlineNote = noteData;
} else {

noteData.m_provider->inlineNoteMouseMoveEvent(KTextEditor::InlineNote(noteData),
 e->globalPos());
}
} else if (m_activeInlineNote.m_position.isValid()) {
tagLine(m_activeInlineNote.m_position);
updateDirty();
m_activeInlineNote.m_underMouse = false;

m_activeInlineNote.m_provider->inlineNoteFocusOutEvent(KTextEditor::InlineNote(m_activeInlineNote));
m_activeInlineNote = {};
}
}

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch added a comment.


  Then it seems like the line is not tagged correctly. Maybe try 
`tagLines(note.position.line(), note.position.line())`? I think the column 
being the same is not what the function being called expects.

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch added a comment.


  My guess is right now the view updates when the cursor blinks, so that's why 
it updates after a short moment (of varying length, though, if you look at the 
video). Since the line is tagged dirty, it gets repainted correctly, but too 
late.

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch added a comment.


  Hm, yeah, looking at the code I think you might need to call `updateView()` 
in case a focus in or out happened. Can you try if that makes a difference?

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D26840: Correctly set underMouse() for inline notes

[Sven Brauch]

brauch added a comment.


  Is the video the new behaviour? It still looks a bit weird to me, there is a 
slight delay between the mouse entering the area and the highlight changing.
  
  Is it possible that the line (and thus the note) is not properly marked for 
repaint when the underMouse property changes?

REPOSITORY
  R39 KTextEditor

BRANCH
  underMouse (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26840

To: davidre, #ktexteditor, cullmann
Cc: brauch, cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

D22511: Minimap: Do not grab left mouse click over up/down arrows

[Sven Brauch]

brauch accepted this revision.
brauch added a comment.
This revision is now accepted and ready to land.


  LGTM, thank you!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D22511

To: sars, brauch
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, sbergeron, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D22477: With auto completion don't show completions that don't match from beginning of typed word

[Sven Brauch]

brauch added a comment.


  Also here, looks good to me, but I would wait for feedback from somebody else 
in addition. Thank you for working on this!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D22477

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, kossebau, mwolff, 
kfunk
Cc: brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, sbergeron, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D22477: With auto completion don't show completions that don't match from beginning of typed word

[Sven Brauch]

brauch added inline comments.

INLINE COMMENTS

> brauch wrote in katecompletionmodel.cpp:2029
> Maybe you want to set the flag here too?

Actually no, probably not. ;)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D22477

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, kossebau, mwolff, 
kfunk
Cc: brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, sbergeron, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D22477: With auto completion don't show completions that don't match from beginning of typed word

[Sven Brauch]

brauch added inline comments.

INLINE COMMENTS

> katecompletionmodel.cpp:2029
>  if (matchesAbbreviation(m_nameColumn, match, 
> model->matchCaseSensitivity())) {
>  matchCompletion = AbbreviationMatch;
>  }

Maybe you want to set the flag here too?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D22477

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, kossebau, mwolff, 
kfunk
Cc: brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, sbergeron, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D22500: Make keyword completion model return HideListIfAutomaticInvocation by default

[Sven Brauch]

brauch added a comment.


  Independent of anything else I think this is a very sensible change, and 
seems like an oversight / bug.
  
  One of the more core kate guys should approve, but +1 from me.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D22500

To: ahmadsamir, #ktexteditor, #kdevelop, cullmann, dhaumann, brauch
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, sbergeron, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D22477: Add a config to only show completions matching word beginning

[Sven Brauch]

brauch added a comment.


  I'm not sure if this solves the right problem.
  
  Where I notice this issue a lot is when typing "return". The keyword 
completion suggests "return", and when I want a newline, I complete the 
"return" first. This doesn't affect C++ (because of the trailing ";"), but it 
does affect other languages.
  
  I think this is pretty much the same issue. Maybe it should be solved by 
hiding the completion window if it was not explicitly invoked if there is any 
entry matching what you currently typed?
  
  Then typing Foo would not have the completion window open at all in the 
described case (thus solving your problem, as well as mine), and if you still 
wanted to complete BarFoo, you would press Ctrl+Space.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D22477

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, kossebau, mwolff, 
kfunk
Cc: brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, sbergeron, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D20852: Fix to show folding preview when move the mouse from bottom to top

[Sven Brauch]

brauch added a comment.


  I guess the intention of the highlight delay is that when you move your mouse 
across the border, the view doesn't flicker. The 150ms does this well enough 
for me, I never see a flicker at least ;)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20852

To: loh.tar, #ktexteditor
Cc: brauch, kwrite-devel, kde-frameworks-devel, #ktexteditor, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D19764: Fix Minimap with QtCurve style

[Sven Brauch]

brauch accepted this revision.
brauch added a comment.
This revision is now accepted and ready to land.


  Looks sensible, and I think I've even seen this bug already somewhere.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19764

To: sars, brauch, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Sven Brauch]

brauch added a comment.


  Sorry, yeah, I misunderstood!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Sven Brauch]

brauch added a comment.


  Hm, so shift-selecting with Shift+Left 5 times, then pressing Shift+Right 
once clears the selection? Just for the statistics, deselecting one character 
is a feature I use all the time.
  
  Why is this behaviour helpful? If you want to clear the selection, press Esc.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D17459: SearchBar: Add Cancel button to stop long running tasks

[Sven Brauch]

brauch added a comment.


  I would also advise against calling processEvents() to keep UIs responsive in 
all cases. It's tempting, but it is near impossible to get it right. What about 
conflicting actions, close/resize events, dbus calls, etc etc that are handled 
here?
  
  I think the right way to implement this would be something like, copy the 
required data, run the expensive S with QtConcurrent, and replace it back 
into the UI when done.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: brauch, cullmann, abetts, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
hase, michaelh, ngraham, bruns, demsking, sars, dhaumann

D17241: Disable highlighting for lines longer than 1024 characters.

[Sven Brauch]

brauch added a comment.


  Shouldn't this change simultaneously remove the line length limit ...?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause
Cc: brauch, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch added a comment.


  For the record, I tried writing a test for this but didn't succeed and 
eventually put it aside, although the difference is easily visible in a test 
application. There must be a reason why the naive test case behaves differently 
from an interactive application ... I could take another look, I guess.

REVISION DETAIL
  https://phabricator.kde.org/D7010

To: brauch, cfeck, dfaure
Cc: dfaure, dhaumann, aacid, #frameworks, michaelh, ngraham, bruns

D14897: InlineNote: Pimpl inline note data without allocs

[Sven Brauch]

brauch added a comment.


  Sorry, never mind -- that code I removed yesterday.  All should be fine.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14897

To: dhaumann, cullmann
Cc: brauch, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D14897: InlineNote: Pimpl inline note data without allocs

[Sven Brauch]

brauch added a comment.


  Looks ok to me, except one thing: the operator== is used to compare a note 
from the list to the "currently active" note in the view. If this compares also 
the "under mouse" state, this code might be broken now ...?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14897

To: dhaumann, cullmann
Cc: brauch, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D14894: [RFC] Fix block mode for multi-cursor branch

[Sven Brauch]

brauch added a comment.


  Thanks for the patch, the approach looks reasonable at a first glance. You 
might want to unite the up/down functions ...
  It is too much of a WiP to merge it like this, though.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14894

To: lepagevalleeemmanuel, brauch
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch closed this revision.
brauch added a comment.


  commit 4ea5fee0afe5c76bbee07563c23ede808aa059de
  Author: Sven Brauch 
  Date:   Tue Aug 14 12:31:31 2018 +0200
  
Add inline note interface

Original patch by Michal Srb.

The inline note interface provides a way to render arbitrary things in
the text. The layout of the line is adapted to create space for the note.

Possible applications include showing a name of a function parameter on call
side or rendering square with color preview next to CSS color property.

Subscribers: kwrite-devel, kde-frameworks-devel

Tags: #kate, #frameworks

Differential Revision: https://phabricator.kde.org/D14826

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch updated this revision to Diff 39889.
brauch added a comment.


  update license text

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39888=39889

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenote.h
  src/include/ktexteditor/inlinenoteinterface.h
  src/include/ktexteditor/inlinenoteprovider.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch updated this revision to Diff 39888.
brauch marked 21 inline comments as done.
brauch added a comment.


  Implement Dominik's suggestions

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39815=39888

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenote.h
  src/include/ktexteditor/inlinenoteinterface.h
  src/include/ktexteditor/inlinenoteprovider.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch updated this revision to Diff 39815.
brauch added a comment.


  address Dominik's suggestion and split focus handling and click handling

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39802=39815

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  CMakeLists.txt
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch updated this revision to Diff 39802.
brauch added a comment.


  I added the rest of the interaction interface (click, mouseover)
  and reduced the API a bit by moving a few hints into the InlineNote
  object.
  
  Only thing I still intend to change about the API would be that
  we use the Qt mouse button enum to provide details about mouse events,
  otherwise nothing comes to mind.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39721=39802

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  CMakeLists.txt
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D5802: ViewPrivate, KateSearchBar, KateVi::MatchHighlighter: use selection foreground for search highlights

[Sven Brauch]

brauch added a comment.
Herald added a project: Kate.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.


  Can't we simply update our shipped schemas, and expect users with custom 
schemas to fix them?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D5802

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kde-frameworks-devel, brauch, dhaumann, mwolff, kwrite-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars, #frameworks

D14826: inline note interface wip #2

[Sven Brauch]

brauch updated this revision to Diff 39721.
brauch added a comment.


  add noteActivated notifier function
  
  When a note is mouse-overed or clicked, a function on the note
  provider is called, giving the point it was hovered or clicked
  in note coordinates, the type of event, and the note the event
  occured on.
  
  This can be used to e.g. expand notes on mouse-over, show a tooltip,
  or execute actions when clicking. You should even be able to e.g.
  highlight pseudo-buttons mouseover.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39692=39721

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  CMakeLists.txt
  src/document/katedocument.h
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch added a comment.


  Thanks for the feedback! I will try doing a few more things with this 
interace and then maybe discuss again with the other kate people here at 
Akademy about which one they like better.
  
  About the tracking, I don't think anything is needed on the side of the 
interface. You can see how you can potentially do it in the KDevelop patch I 
attached: create a moving cursor for the location you want the note to track, 
and then compute the note's position from the moving cursor's position as 
needed in the getter each time.
  I think this is even better than doing it in the interface itself, since it 
is more flexible with regards to how exactly the moving cursor behaves.
  
  Regarding the QVarLengthArray, performance-wise it would be better, but it 
makes the public API ugly (QVarLengthArray is a relatively internal, hacky 
class which is supposed to be only used in special cases), so I think we should 
first profile whether this is a bottleneck in any possible use case (it 
probably isn't).
  
  Currently, I'm trying out whether we can add some simple interaction ("note 
clicked") as well already, since I think that would be nice long-term. If the 
API is nice, we can fix small uglyness like the cursor navigation around notes 
at any later time IMHO.
  
  Best,
  Sven

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch updated this revision to Diff 39692.
brauch added a comment.


  add missing files

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39682=39692

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch added a comment.


  Sample patch for KDevelop's problem highlighter plus screenshot:
  F6192637: hl.png 
  F6192639: inline-problems.diff 

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch added reviewers: michalsrb, dhaumann, cullmann.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

[Sven Brauch]

brauch created this revision.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
brauch requested review of this revision.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/render/katerenderer.cpp
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h

To: brauch
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  I'd like to play with this a bit wrt what can be done in KDevelop with it (I 
want the problem popups gone). Would you mind if I do some changes along the 
way? I would post an updated patch here, in case I actually come up with useful 
changes ...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, cullmann, ngraham, brauch, 
michaelh, kevinapavew, bruns, demsking, sars

D14758: use view lines for wheel scrolling, not real lines

[Sven Brauch]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:2815fea7854f: Fix: Scroll view lines instead of real lines 
for wheel and touchpad scrolling (authored by brauch).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D14758?vs=39499=39561#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14758?vs=39499=39561

REVISION DETAIL
  https://phabricator.kde.org/D14758

AFFECTED FILES
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, dhaumann
Cc: rjvbb, cullmann, ngraham, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D14773: completion widget: fix minimum header section size

[Sven Brauch]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:5499a0df825c: completion widget: fix minimum header 
section size (authored by brauch).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14773?vs=39533=39562

REVISION DETAIL
  https://phabricator.kde.org/D14773

AFFECTED FILES
  src/completion/expandingtree/expandingtree.cpp

To: brauch, #kate, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  By the way, other people around here are very impressed by this patch as 
well, and we'd really like to get this merged :)
  Moving e.g. KDevelop's warning markers into an inline note instead of the 
annoying popup would make a real difference for usability ...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, cullmann, ngraham, brauch, 
michaelh, kevinapavew, bruns, demsking, sars

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  Wow, that looks amazing! Really impressive.
  
  Using moving cursors for this sounds good to me. Other things which cannot be 
updated in real-time do that as well (highlighting, error underlines, ...)
  
  Regarding the cursor issue: I think you would need a state variable whether 
the cursor is left or right of the note. You could then patch 
KateViewInternal::placeCursor to set this correctly and also the cursor move 
functions to take this into account. That's not ideal since it requires to 
modify a few places, but probably actually adding a character would require 
more modifications...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, cullmann, ngraham, brauch, 
michaelh, kevinapavew, bruns, demsking, sars

D14758: use view lines for wheel scrolling, not real lines

[Sven Brauch]

brauch added a comment.


  In D14758#307401 , @ngraham wrote:
  
  > In D14758#307333 , @brauch wrote:
  >
  > > The "overly sensitive touchpad" issue seems to be missing accumulation of 
scroll events, so this patch to my understanding should not have it.
  >
  >
  > Yeah, I don't think that's our bug, and we shouldn't work around it here. 
The reporter should file a bug against libinput.
  
  
  Hmm, I understood this differently. I thought this happens when you react to 
each input event sent by the kernel, regardless of its delta, like e.g. the old 
gwenview code did which you linked: it just zooms in once per event. Thus 
devices which generate more events with smaller deltas zoom in faster, which is 
not good. And this bug is definitely at the application level, not in libinput.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14758

To: brauch, dhaumann
Cc: ngraham, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, bruns, 
demsking, cullmann, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.
Restricted Application edited subscribers, added: kde-frameworks-devel, 
kwrite-devel; removed: Frameworks.


  Hey there, what's happening to this? I think this is a really nice feature 
and it would be very sad if it would bitrot :(
  Anything anyone can assist with? We are currently at Akademy, KDE's annual 
conference, so we would have time to discuss any issues right now.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, cullmann, ngraham, brauch, 
michaelh, kevinapavew, bruns, demsking, sars, #frameworks

D14758: use view lines for wheel scrolling, not real lines

[Sven Brauch]

brauch added a comment.


  The "overly sensitive touchpad" issue seems to be missing accumulation of 
scroll events, so this patch to my understanding should not have it.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14758

To: brauch, dhaumann
Cc: ngraham, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, bruns, 
demsking, cullmann, sars, dhaumann

D14758: use view lines for wheel scrolling, not real lines

[Sven Brauch]

brauch added a comment.


  I will modify the commit message as suggested.
  
  I read through the bug report, and I think this patch does fix the main issue 
discussed there. The patch suggested by one user there is even quite similar 
(if a bit less complete) than this one. The original complaint (touchpad vs. 
mouse wheel) to me personally sounds a bit strange and I'm not sure if that is 
actually the point. I'm not even sure if that is what the original reporter was 
in fact annoyed by (in contrast to "what he wrote he was annoyed by").

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14758

To: brauch, dhaumann
Cc: ngraham, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, bruns, 
demsking, cullmann, sars, dhaumann

D14773: completion widget: fix minimum header section size

[Sven Brauch]

brauch created this revision.
brauch added a reviewer: Kate.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel.
brauch requested review of this revision.

REVISION SUMMARY
  The header is hidden, but if the minimum size is -1 (the default), Qt
  chooses something non-zero as the minimum size. The completion widget
  then sets it to something, assumes it has that width, and messes up
  its layout when it actually ends up being larger than that.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14773

AFFECTED FILES
  src/completion/expandingtree/expandingtree.cpp

To: brauch, #kate
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14758: use view lines for wheel scrolling, not real lines

[Sven Brauch]

brauch created this revision.
brauch added a reviewer: dhaumann.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel.
brauch requested review of this revision.

REVISION SUMMARY
  I don't think there is an ideal solution for this problem, this one has the 
disadvantage that the code handling wheel scrolling is not related to the 
scroll bar any more. It still seems like the simplest one. What do you think?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14758

AFFECTED FILES
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D13442: Implemented displaying of total lines in kate

[Sven Brauch]

brauch added a comment.


  I was also thinking about the context menu, yes. That is also where a lot of 
users would first look for it I think, even before the config dialog.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13442

To: shubham, #ktexteditor, cullmann, brauch
Cc: mludwig, zhigalin, ngraham, dhaumann, kwrite-devel, kde-frameworks-devel, 
michaelh, kevinapavew, bruns, demsking, head7, cullmann, kfunk, sars

D13442: Implemented displaying of total lines in kate

[Sven Brauch]

brauch added a comment.


  I don't like this being user-controllable. We spend so much effort in keeping 
our option set clearly arranged and limited to useful choices, to me this seems 
like a very bad opportunity to break that rule.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13442

To: shubham, #ktexteditor, cullmann, brauch
Cc: mludwig, zhigalin, ngraham, dhaumann, kwrite-devel, kde-frameworks-devel, 
michaelh, kevinapavew, bruns, demsking, head7, cullmann, kfunk, sars

D13442: Implemented displaying of total lines in kate

[Sven Brauch]

brauch added a comment.


  Since the other two number displays use QLocale().toString(), probaly this 
should too ...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13442

To: shubham, #ktexteditor, cullmann, brauch
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, head7, cullmann, kfunk, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:e6f87dd57008: Fix caret width (authored by shubham, 
committed by brauch).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13365?vs=35683=35735

REVISION DETAIL
  https://phabricator.kde.org/D13365

AFFECTED FILES
  src/render/katerenderer.cpp

To: shubham, #ktexteditor, brauch, cullmann
Cc: cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  I'm sorry I am so annoying, but iirc KDE's commit hooks will not accept 
commits with only one name fragment. Something equivalent to the western 
first/last name pair is required.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor, brauch, cullmann
Cc: cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  To submit the change with your name on it, I'd need a full name and email 
address, can you provide that? Thanks!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor, brauch, cullmann
Cc: cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  Yes that looks better, I'll submit it later today.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor, brauch, cullmann
Cc: cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  Click "download raw diff" at the top-right of the page. This is the diff you 
uploaded. This is not the change we want to apply when merging this review 
request; it only contains the changes you did from your earlier version to the 
latest one, not the set of changes which needs to be applied to the repository. 
If you try to apply this patch to ktexteditor master, you will see that it 
fails.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor, brauch, cullmann
Cc: cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  The change is good now but you screwed up the patch: you need to submit the 
full set of changes you want to do as a patch, not only the last iteration. Can 
you fix that? Thank you!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor, brauch, cullmann
Cc: cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D13365: Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch accepted this revision.
brauch added a comment.
This revision is now accepted and ready to land.


  Yep, with that, it looks good to me. Do you have commit access?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor, brauch
Cc: ngraham, brauch, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
bruns, demsking, cullmann, sars, dhaumann

D13365: BUG:391518 Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  Good find (annoyed me too) and sounds plausible to me. Thanks!
  
  Just one thing, do you call setRenderHint() inside the save() / restore() 
pair of the painter so the previous state is restored after drawing the caret?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor
Cc: brauch, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D13365: BUG:391518 Fixed the cursor(caret) width in kate

[Sven Brauch]

brauch added a comment.


  Good find (annoyed me too) and sounds plausible to me. Thanks!
  
  Just one thing, do you call setRenderHint() inside the save() / restore() 
pair of the painter so the previous state is restored after drawing the caret?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D13365

To: shubham, #ktexteditor
Cc: brauch, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D12768: Allow wrapping selection off top/bottom of autocomplete results

[Sven Brauch]

brauch added a comment.


  What happens if the overload selection window is open in addition (like in 
KDevelop)?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12768

To: sraizada, #ktexteditor
Cc: brauch, #ktexteditor, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, head7, cullmann, kfunk, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added inline comments.

INLINE COMMENTS

> dhaumann wrote in katedocument.cpp:5316-5320
> Btw, may I am wrong here, since this is really called often when painting 
> lines... So this may be good to have.

I also stumbled upon it but it avoids an alloc in a very deep loop, so it might 
be worth it here.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: dhaumann, cullmann, ngraham, brauch, #frameworks, michaelh, kevinapavew, 
bruns, demsking, sars

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  Oh, heh, yes it does. It's just your docstring which says otherwise ("is an 
interface for the View"), and that's what I looked at at that time. That should 
be changed. ;)
  
  I think the question is less what you need and more what it is supposed to do 
semantically. Do you add annotations to a *document*, or do you add them to a 
specific *view* of that document (remember that there can be more than one view 
of a document, even visible at once, e.g. splitview)? I think you can find 
arguments for both, but I feel like the document variant (as you have it) will 
be easier to handle.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: cullmann, ngraham, brauch, #frameworks, michaelh, kevinapavew, bruns, 
demsking, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  I think fixing the selection rendering issue would be nice.
  
  Regarding blockmode behaviour, I think it will be a bit strange either way. 
One thing to compare is the behaviour with non-fixed width fonts: there block 
mode also selects constant columns (i.e. it will look ragged). This is probably 
closest to what you want when you use block mode, so I think it should stay the 
way you have it now.
  
  Regarding linewrap, I think what at least must always be the case is that 
with dynwrap on, all text is visible. That notes cannot be placed nicely is 
probably unavoidable in extreme cases. I guess the best behaviour would be to 
just wrap the notes as if they were part of the text, with the restriction that 
you cannot split them up ...
  
  I would not rely on the user of the interface placing his notes in a visible 
column. If e.g. KDevelop would place some notes, and then the user resizes his 
window, and then they need to be re-placed by the interface user? That seems 
strange. Consider also that you can have a split view with two different widths 
for the same file and stuff like that.
  
  Oh, good point by the way: Are you sure this should be an interface to a 
*view*? Maybe it should instead be attached to the document? I'm not sure, I 
just want to bring the discussion up.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: ngraham, brauch, #frameworks, michaelh, kevinapavew, bruns, demsking, 
cullmann, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  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?
  
  Some things which come to my mind for testing would be:
  
  - is selection rendered correctly if it includes notes, at the end, 
beginning, or middle of lines, also mult-line selections?
  - what happens when clicking or dragging from or into the notes?
  - does it still work properly with dynamic word-wrap on?
  - does it work properly with code-folding? what happens if a note is at the 
border of a folding region?

INLINE COMMENTS

> katedocument.cpp:5295
> +
> +connect(provider, SIGNAL(reset()), this, SLOT(inlineNotesReset()));
> +connect(provider, SIGNAL(lineChanged(int)), this, 
> SLOT(inlineNotesLineChanged(int)));

Can you use new-style connect here, i.e. the function-pointer syntax
connect(provider, ::reset, this, 
::inlineNotesReset)?
This gives compile-time argument type checking.

> katerenderer.cpp:771
> +// Determine the position where to paint the note.
> + // We start by getting the x coordinate of cursor placed to the 
> column.
> +qreal x = 
> range->viewLine(viewLine).lineLayout().cursorToX(column) - xStart;

indent

> katerenderer.cpp:777
> +// note should be painted and the cursor gets placed at the 
> right side of it. So we have to
> + // subtract the width of the note to get to left side of the 
> hole.
> +x -= inlineNote->width(lineHeight(), currentFontMetrics());

indent

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: brauch, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  Awesome idea! Do you have a screenshot of how it looks?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12662

To: michalsrb, #ktexteditor
Cc: brauch, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Sven Brauch]

brauch added a comment.


  Oh and, you do not need to inherit QObject to use connect; you can connect to 
a lambda calling the member function AFAIK or so. Just omit the third argument 
in connect(). What you lose by doing this is the automatic disconnect of the 
connection when the receiver object is deleted, so make sure that doesn't 
happen.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12016

To: jtamate, #kate, cullmann, #frameworks
Cc: brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Sven Brauch]

brauch added a comment.


  Re. binary compatibility: should be fine because this class is not exported 
(no KTEXTEDITOR_EXPORT macro).

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12016

To: jtamate, #kate, cullmann, #frameworks
Cc: brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D11838: Turn on line numbers by default

[Sven Brauch]

brauch added a comment.


  For KDevelop this is fine, I don't think we have any objection.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D11838

To: ngraham, #kate, #ktexteditor, dhaumann, mludwig
Cc: brauch, mludwig, kfunk, dhaumann, #frameworks, michaelh, kevinapavew, 
ngraham, demsking, cullmann, sars

D11811: avoid Asan runtime error: shift exponent -1 is negative

[Sven Brauch]

brauch added a comment.


  Change looks good (the previous code definitely looks like nonsense), but 
what does this mean for existing settings, saved previously?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D11811

To: jtamate, #kate, #frameworks
Cc: brauch, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, dhaumann

D9569: Fix wildcard matching for modelines

[Sven Brauch]

brauch accepted this revision.
brauch added a comment.
This revision is now accepted and ready to land.


  Heh. Fix looks obviously correct to me (good find), and tests are always nice.

REPOSITORY
  R39 KTextEditor

BRANCH
  TestModelines (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9569

To: dhaumann, cullmann, kfunk, mwolff, nalvarez, brauch
Cc: brauch, #frameworks, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D7660: Fix a regression caused by changing backspace key behavior

[Sven Brauch]

brauch reopened this revision.
brauch added inline comments.

INLINE COMMENTS

> katedocument_test.cpp:452
>  auto view = 
> static_cast(doc.createView(nullptr));
> +view.config()->setBackspaceRemoveComposed(true);
>  doc.setText(QString::fromUtf8("व्यक्तियों"));

must be view->config ... how did this patch ever compile for you??

> katedocument.cpp:3190
> +}
> +if (!config()->backspaceIndents() || pos) {
> +KTextEditor::Cursor beginCursor(line, 0);

pos is not defined at this place

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D7660

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann, cullmann
Cc: brauch, mwolff, ngraham, anthonyfieroni, cullmann, jgrulich, dhaumann, 
hein, kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Sven Brauch]

brauch added a comment.


  After this was submitted master doesn't compile for me, and if I fix the 
compile in the trivial way the test fails. Can you have another look?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D7660

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann, cullmann
Cc: brauch, mwolff, ngraham, anthonyfieroni, cullmann, jgrulich, dhaumann, 
hein, kwrite-devel, #frameworks

D8333: fix some indenters from randomly invoking indent

[Sven Brauch]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:aeebeadb5f59: fix some indenters from indenting on random 
characters (authored by brauch).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8333?vs=20861=20874

REVISION DETAIL
  https://phabricator.kde.org/D8333

AFFECTED FILES
  src/script/kateindentscript.cpp

To: brauch, #ktexteditor, cullmann, dhaumann
Cc: anthonyfieroni, #frameworks, kevinapavew, cullmann, sars, dhaumann

D8333: fix some indenters from randomly invoking indent

[Sven Brauch]

brauch marked 2 inline comments as done.
brauch added a comment.


  Yes, at least the ones I'm aware of. Thanks for the review, I'll push it in a 
moment.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D8333

To: brauch, #ktexteditor, cullmann, dhaumann
Cc: anthonyfieroni, #frameworks, kevinapavew, cullmann, sars, dhaumann

D8333: fix some indenters from randomly invoking indent

[Sven Brauch]

brauch marked an inline comment as done.
brauch added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kateindentscript.cpp:47
> But if triggerCharacters are undefined this variable should be false, no?

No, that's still fine, this variable just caches reading the value from the 
script object. If the script doesn't define it, it is read as empty, and then 
doesn't look again (because that won't change in the future).

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D8333

To: brauch, #ktexteditor, cullmann, dhaumann
Cc: anthonyfieroni, #frameworks, kevinapavew, cullmann, sars, dhaumann

D8333: fix some indenters from randomly invoking indent

[Sven Brauch]

brauch created this revision.
brauch added reviewers: KTextEditor, cullmann, dhaumann.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  If triggerCharacters was not set, toString() would return "undefined",
  making indenters trigger on u, n, d, e, f, i and n.

TEST PLAN
  Trigger chars are still set correctly for e.g. cmake.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D8333

AFFECTED FILES
  src/script/kateindentscript.cpp

To: brauch, #ktexteditor, cullmann, dhaumann
Cc: #frameworks, kevinapavew, cullmann, sars, dhaumann

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch added a comment.


  I can write a test if you think this helps. I think reading the docs it is 
quite clear we must call updateGeometry() here: our sizeHint() changes when 
changing the text.

REVISION DETAIL
  https://phabricator.kde.org/D7010

To: brauch, cfeck, rkflx
Cc: dhaumann, aacid, #frameworks

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch updated this revision to Diff 17638.
brauch added a comment.


  Right. Better call it here.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7010?vs=17411=17638

REVISION DETAIL
  https://phabricator.kde.org/D7010

AFFECTED FILES
  src/ksqueezedtextlabel.cpp

To: brauch, cfeck
Cc: aacid, #frameworks

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch added a comment.


  With enough dedication, probably yes ...

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7010

To: brauch, cfeck
Cc: aacid, #frameworks

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  By changing the full text, the sizeHint() of the widget changes. If the   

   
  text is currently squeezed, this however does not imply that the text 

   
  passed to QLabel::setText() changes (which is a no-up if not). Thus we need   

   
  to call updateGeometry() to notify the layout system about the size change.   

   
  Otherwise, the label misbehaves when you e.g. set the size policy to  

   
  Minimum and place it next to a spacer with Expanding policy, and then 

   
  change the text.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7010

AFFECTED FILES
  src/ksqueezedtextlabel.cpp

To: brauch, cfeck
Cc: #frameworks

D6870: add missing standard types to C highlighting and update to C11

[Sven Brauch]

This revision was automatically updated to reflect the committed changes.
Closed by commit R216:c40ad3513f91: add missing standard types to C 
highlighting and update to C11 (authored by brauch).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D6870?vs=17104=17109#toc

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6870?vs=17104=17109

REVISION DETAIL
  https://phabricator.kde.org/D6870

AFFECTED FILES
  data/syntax/c.xml
  data/syntax/isocpp.xml

To: brauch, vkrause, dhaumann
Cc: vkrause, dhaumann, #frameworks

D6870: add missing standard types to C highlighting and update to C11

[Sven Brauch]

brauch added reviewers: vkrause, dhaumann.

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D6870

To: brauch, vkrause, dhaumann
Cc: vkrause, dhaumann, #frameworks

D6870: add missing standard types to C highlighting and update to C11

[Sven Brauch]

brauch created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  See bug: https://bugs.kde.org/show_bug.cgi?id=367798
  
  My suggestion is to add the new C11 keywords to the C highlighting, add the 
missing standard types (like time_t) to both C and iso-C++, and ignore the 
POSIX stuff.

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D6870

AFFECTED FILES
  data/syntax/c.xml
  data/syntax/isocpp.xml

To: brauch
Cc: vkrause, dhaumann, #frameworks

D4234: Change algorithm for autobrace.

[Sven Brauch]

brauch added a comment.


  Hm, I think the biggest problem is that we don't know how we want it to work 
... so we can't write tests either ;)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4234

To: cactus, #ktexteditor, mwolff
Cc: mwolff, anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, 
#frameworks

[Differential] [Commented On] D4538: [KTextEditor] consistent conversion from/to cursor to/from coordinates

[Sven Brauch]

brauch added a comment.


  Looks sensible to me, but can you check that it doesn't break KDevelop's 
navigation widget? I think that is the most visible use case for that interface 
right now.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4538

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: jsalatas, #frameworks, #plasma, #ktexteditor
Cc: brauch, cullmann, plasma-devel, kwrite-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4234: Change algorithm for autobrace.

[Sven Brauch]

brauch added a comment.


  In https://phabricator.kde.org/D4234#86372, @anthonyfieroni wrote:
  
  > You mean when we have 3 open when 1 is close to be inserted 2 more to 
balance counting ? It's not a good idea, about me.
  
  
  No, but if you type a closing one right before a closing one, "eat" it iff 
there's no closing one missing right now.
  
  I don't think the suggested behaviour is worse than what we have now, but 
it's also not better IMO, so I'm not in favour of changing it ...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4234

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: cactus, #ktexteditor
Cc: anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, #frameworks

[Differential] [Commented On] D4234: Change algorithm for autobrace.

[Sven Brauch]

brauch added a comment.


  Sorry, I wanted to write a reply but failed. My idea was simply to have the 
algorithm always aim to make parentheses balanced when closing one.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4234

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: cactus, #ktexteditor
Cc: dhaumann, brauch, cullmann, kwrite-devel, #frameworks

[Differential] [Commented On] D4234: Change algorithm for autobrace.

[Sven Brauch]

brauch added a comment.


  Let's discuss an alternative suggestion maybe: how about it just keeps track 
of the parenthesis balancing and removes them if doing so would make it 
unbalanced? It could stop counting at the next folding region, and exclude 
spellchecked parts.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4234

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: cactus, #ktexteditor
Cc: brauch, cullmann, kwrite-devel, #frameworks

[Differential] [Commented On] D4234: Change algorithm for autobrace.

[Sven Brauch]

brauch added a comment.


  I'm a bit skeptical, this means I can't add closing braces when they are 
missing ...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4234

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: cactus, #ktexteditor
Cc: brauch, cullmann, kwrite-devel, #frameworks

Re: Minimum required cmake version

[Sven Brauch]

Hey,

thanks for the discussion. I've changed the required cmake version to
3.0 for all frameworks, hope I didn't miss any.

Best regards,
Sven

On 27/12/16 17:48, Sven Brauch wrote:
> Hi,
> 
> building on CentOS 6.8, I noticed some frameworks (namely KAuth,
> KSyntaxHighlighting) don't actually build with cmake 2.8 -- it seems to
> be related with moc'ing the Q_GADGET macro. Still, they specify cmake
> 2.8 as the minimum required cmake version.
> 
> Do we
>  - attempt to fix this with cmake 2.8
>  - bump the required cmake version to 3.0 for these frameworks
>  - bump the required cmake version to 3.0 for all frameworks?
> 
> Greetings,
> Sven
> 




signature.asc
Description: OpenPGP digital signature

Re: Minimum required cmake version

[Sven Brauch]

On 03/01/17 14:13, David Faure wrote:
>> No objection.
> 
> I do, however, have an objection to untested commits.
> 
> Sven, your commits broke knewstuff and some other modules.
> 
> CMake Error at /d/kde/inst/kde_frameworks/share/ECM/modules/
> ECMGeneratePriFile.cmake:137 (message):
>   Required variable PROJECT_VERSION_STRING not set before
>   ECM_GENERATE_PRI_FILE() call.  Did you call ecm_setup_version?
> Call Stack (most recent call first):
>   src/core/CMakeLists.txt:94 (ecm_generate_pri_file)
> 
> Changing the min required cmake version also changes the default values for a 
> number of cmake policies, so you need to adjust the cmakelists.txt 
> accordingly.

I'm sorry. I completely forgot changing the minimum cmake version
changes these defaults (and thus actually affects behaviour). I'll look
into it right now.





signature.asc
Description: OpenPGP digital signature

Re: Minimum required cmake version

[Sven Brauch]

Hey,

On 03/01/17 14:13, David Faure wrote:
> Sven, your commits broke knewstuff and some other modules.
> 
> CMake Error at /d/kde/inst/kde_frameworks/share/ECM/modules/
> ECMGeneratePriFile.cmake:137 (message):
>   Required variable PROJECT_VERSION_STRING not set before
>   ECM_GENERATE_PRI_FILE() call.  Did you call ecm_setup_version?
> Call Stack (most recent call first):
>   src/core/CMakeLists.txt:94 (ecm_generate_pri_file)
> 
> Changing the min required cmake version also changes the default values for a 
> number of cmake policies, so you need to adjust the cmakelists.txt 
> accordingly.
I ported everything to the new default policy settings and also fixed
several cmake warnings the switch to 3.0 introduced. The only thing I
don't know how to port is one place in sonnet, there I just set the old
policy for now.

Apologies once more for the noise and breakage!

Best,
Sven




signature.asc
Description: OpenPGP digital signature

Re: Minimum required cmake version

[Sven Brauch]

Hey,

On 27/12/16 19:30, Aleix Pol wrote:
> +1 on changing to 3.0.
Thanks for your opinion on the matter. I also think we should just bump
it, 3.0 is old enough -- as said it's even easily available on CentOS 6.

Question is, should I change it on _all_ frameworks or just those using
Q_GADGET (i.e. those which actually need cmake 3)?

Best,
Sven



signature.asc
Description: OpenPGP digital signature

Re: Minimum required cmake version

[Sven Brauch]

On 27/12/16 19:23, Aleix Pol wrote:
> Makes sense, although you still won't be able to compile these
> frameworks on CentOS 6.8 even if we fix that, right?
I have CMake 3.6 on CentOS 6.8, it's just called "camke3" instead of
"cmake". That's why I was running cmake 2.8 so far, and noticed this issue.

It doesn't prevent me from compiling, it just doesn't make sense to
specify 2.8 as the minimum if it doesn't actually work (and was
confusing to find out).

Sven



signature.asc
Description: OpenPGP digital signature

Re: Minimum required cmake version

[Sven Brauch]

Hi,

On 27/12/16 19:18, Aleix Pol wrote:
> Have you checked that changing to cmake 3.0 actually solves this
> problem?
I have 3.6 on the same system and with that it works. I don't have 3.0. :/

However, 3.0 docs explicitly mentions Q_GADGET.

Greetings,
Sven



signature.asc
Description: OpenPGP digital signature