D23721: Fix race condition in TextBuffer

[Thomas Schöps]

thomassc abandoned this revision.
thomassc added a comment.


  Okay, thanks for your feedback. Discarding this then. I think I might have 
found the problematic access in KDevelop already in the meantime ...

REPOSITORY
  R39 KTextEditor

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

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

D23721: Fix race condition in TextBuffer

[Thomas Schöps]

thomassc added a comment.


  Okay, I wasn't aware of the overall situation wrt. threading. Still, couldn't 
it be useful to at least allow for concurrent read accesses which is probably 
not a large effort (in contrast to concurrent read and write)? I guess that 
people might not expect blockForLine() to not be re-entrant, and it seems like 
the proposed robustification might not add too much overhead for performance 
and code complexity.

REPOSITORY
  R39 KTextEditor

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

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

D23721: Fix race condition in TextBuffer

[Thomas Schöps]

thomassc edited the summary of this revision.
thomassc added a reviewer: KTextEditor.

REPOSITORY
  R39 KTextEditor

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

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

D23721: Fix race condition in TextBuffer

[Thomas Schöps]

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

REVISION SUMMARY
  In the "shortcut: try last block first" branch in TextBuffer::blockForLine(),
  m_lastUsedBlock is accessed multiple times. If another tread simultaneously
  executes the binary block search below in the same function, then
  m_lastUsedBlock may be modified by that thread. This may result in the first
  thread returning a wrong block, since it may return the modified value of
  m_lastUsedBlock instead of the original value that it used to determine 
whether
  the given line is in the block.
  
  This change is intended to fix a crash where an out-of-bounds line is accessed
  in katetextblock.cpp, which occurs about one or two times per month for me 
when
  using KDevelop. I have no idea whether this possible race condition is 
actually
  the reason for these crashes, but it seems like a plausible candidate.

TEST PLAN
  I only tested that the code still works after the change.

REPOSITORY
  R39 KTextEditor

BRANCH
  fix_katetextbuffer_race_condition

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

AFFECTED FILES
  src/buffer/katetextbuffer.cpp
  src/buffer/katetextbuffer.h

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

D22315: Add setting to enable/disable text drag-and-drop (on by default)

[Thomas Schöps]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:c0321472330b: Add setting to enable/disable text 
drag-and-drop (on by default) (authored by thomassc).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22315?vs=61299=61359

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

AFFECTED FILES
  src/dialogs/editconfigwidget.ui
  src/dialogs/katedialogs.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h
  src/view/kateviewinternal.cpp

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

D22315: Add setting to enable/disable text drag-and-drop (on by default)

[Thomas Schöps]

thomassc added a comment.


  Thanks for having a look. Here is a screenshot of the config page with the 
new option on it (third from the bottom):
  
  F6991601: kate-config-text-drag-and-drop.png 

  
  Do you think that the label there is okay as-is? Or should it perhaps rather 
be something like "Enable moving selected text by drag and drop" to be more 
clear?

REPOSITORY
  R39 KTextEditor

BRANCH
  text_drag_and_drop_setting

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

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

D22315: Add setting to enable/disable text drag-and-drop (on by default)

[Thomas Schöps]

thomassc added a reviewer: KTextEditor.

REPOSITORY
  R39 KTextEditor

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

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

D22315: Add setting to enable/disable text drag-and-drop (on by default)

[Thomas Schöps]

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

REVISION SUMMARY
  This change adds a setting to enable or disable the ability to move the 
selected text by drag-and-drop. It is on by default, which means the default 
behavior is not changed. Disabling this setting is useful for people like me 
who generally use Ctrl-X/V for moving text. For those, the drag-and-drop is 
never intended, but sometimes triggered accidentally, so it is good to be able 
to remove this pitfall.
  
  FEATURE: 391450

TEST PLAN
  I tested it manually.

REPOSITORY
  R39 KTextEditor

BRANCH
  text_drag_and_drop_setting

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

AFFECTED FILES
  src/dialogs/editconfigwidget.ui
  src/dialogs/katedialogs.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h
  src/view/kateviewinternal.cpp

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

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

[Thomas Schöps]

thomassc added a comment.


  I guess the previous situation is still more practical, since by temporarily 
adding a newline there below the oversized line, it is at least possible to 
edit that line. With the line cut off, it is impossible to edit. Also, in cases 
where a line is only slightly oversized (as with the emoji example), with the 
overpainting it might look just fine, whereas with the cutoff it won't look 
fine.
  
  FYI, below is a screenshot of how the new example looks on my system 
((K)ubuntu 14.04, Monospace font, normal style, size 9, which I believe is the 
default), after applying the line height fix: it looks perfectly fine.
  
  F6649169: bug_404713_monospace_ubuntu.png 


REPOSITORY
  R39 KTextEditor

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

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

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

[Thomas Schöps]

thomassc added a comment.


  > If you keep the overpainting, you will just overpaint the next line 
partially with it, that won't help that much, IMHO.
  > 
  > Take a look at bug 404713.
  
  I totally agree, but this is a different issue that is out of scope here as 
far as I see. Bugs 403868 and 403470 can and should be fixed by the proposed 
line height change. This will not fix the other bugs 404713 and 404907 though, 
which are a different issue and unaffected by this. Thus I think that the 
overpainting workaround should be kept until a better solution is found for the 
latter bugs.

REPOSITORY
  R39 KTextEditor

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

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

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

[Thomas Schöps]

thomassc added a comment.


  > The summary of this diff reads a bit as if it doesn't noticeably improve 
behaviour
  
  The line height change (a part of this commit) does noticeably improve 
behavior for the font that is used on my system.

REPOSITORY
  R39 KTextEditor

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

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

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

[Thomas Schöps]

thomassc added a comment.


  > We trade one bug for another. Which one is worse?
  
  Without having had a detailed look, it seems to me like the problem reported 
by @rjvbb might be caused by the removal of the workaround with the second 
drawing passes in this commit (the changes in src/view/kateviewinternal.cpp), 
which probably results in more issues with oversized characters. This is 
independent from the other change (in src/render/katerenderer.cpp), which 
adapts the line height and fixes the bug with the bottom of lines being cut 
off. So, one might try to revert the removal of the second drawing passes, but 
keep the change to the line heights.

REPOSITORY
  R39 KTextEditor

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

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

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

[Thomas Schöps]

thomassc added a comment.


  I think that this change will fix bug 403470 as well.
  
  Suggestion: in updateFontHeight()  in katerenderer.cpp, add a comment saying 
that the line height needs to match that which is used by Qt, since Qt may 
handle some parts of the drawing, e.g. selection backgrounds, and a mismatch 
will cause drawing issues. Perhaps also say where in the Qt code this can be 
looked up.

REPOSITORY
  R39 KTextEditor

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

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

D17932: Improvements to completion

[Thomas Schöps]

thomassc updated this revision to Diff 49397.
thomassc added a comment.


  Change to a single setter for match_cs and exact_match_cs to avoid 
restriction on call order of functions

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17932?vs=49343=49397

BRANCH
  improvements_to_completion (branched from master)

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann

D17932: Improvements to completion

[Thomas Schöps]

thomassc updated this revision to Diff 49343.
thomassc added a comment.


  Update according to Milian's comments

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17932?vs=48574=49343

BRANCH
  improvements_to_completion (branched from master)

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, head7, kfunk, sars, dhaumann

D17932: Improvements to completion

[Thomas Schöps]

thomassc marked 4 inline comments as done.
thomassc added a comment.


  Thanks for reviewing. Regarding the question about which models would have an 
insensitive exact match, and which ones have sensitive exact matches:
  
  - An example for case-insensitive exact matches might be plain text, or a 
hypothetical case-insensitive programming language. For example for plain text, 
one might want to treat words like "Question" and "question" as exact matches, 
which will make the completion widget close itself when it shows one of them 
and the user types the other. This is the current behavior for the word 
completion in KWrite / Kate.
  - An example for case-sensitive exact matches would be a case-sensitive 
programming language like C++. If the user typed "m_var" but the variable is 
actually called "m_Var", then the completion widget should not hide itself, 
since it might still be useful to replace the typed text with the completion 
item that has different case.

INLINE COMMENTS

> mwolff wrote in katecompletionmodel.cpp:2026
> maybe simplify this to:
> 
> if (matchCompletion && m_nameColumn.startsWith(match, 
> model->exactMatchCaseSensitivity())) {
> 
>   matchCompletion = PerfectMatch;
>   m_haveExactMatch = true;
> 
> }

Hmm, I guess the current version might be a tiny bit faster in general since it 
only does another startsWith() check if this would be a stricter check than the 
one done before ...

> mwolff wrote in katecompletionmodel.h:394
> should this comment be asserted in the setters or is it handled gracefully in 
> the logic below?

I added asserts to the setters. One should be aware then though that it 
restricts the order in which these must be called. For example, if both 
settings are case-insensitive at first and both should be changed to 
case-sensitive, then the exact-match-sensitivity must be changed before the 
match-sensitivity. Alternatively, one could make a single setter function that 
changes both properties.

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, head7, kfunk, sars, dhaumann

D17932: Improvements to completion

[Thomas Schöps]

thomassc created this revision.
thomassc added a reviewer: KTextEditor.
thomassc added a project: KTextEditor.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
thomassc requested review of this revision.

REVISION SUMMARY
  My goal is to fix two issues with code completion in KDevelop:
  
  First, it seems to be mostly insensitive to upper/lowercasing, which is an 
issue with a case-sensitive language such as C++. I'd like to have all 
(case-insensitive) matches in the completion list, but always sort those to the 
top and have them selected by default which match the case of the typed text.
  
  Second, I'd like to have the completion widget hidden when there is a 
case-sensitive exact match, such that the completion widget does not obstruct 
the view in this case. Currently it never seems to get hidden automatically.
  
  This diff is an attempt to implement a first step towards these goals by 
improving some things in KTextEditor. I'd make more changes to ensure that the 
auto-selected completion item tries to match the case of the typed text, and to 
make this usable in KDevelop, but would like to put this up here for discussion 
first before making further changes. Would you be fine with the proposed 
changes in general? Would you choose a different approach? The changes in this 
initial diff are described below.
  
  1. The diff introduces a new setting m_exactMatchCaseSensitivity next to 
m_matchCaseSensitivity. By setting m_matchCaseSensitivity == 
Qt::CaseInsensitive and m_exactMatchCaseSensitivity == Qt::CaseSensitive, it is 
possible to have all case-insensitive matches in the completion list, while 
only allowing case-sensitive matches to be exact matches (that will hide the 
completion widget).
  2. In KateCompletionModel::Item::operator <, the (case-sensitive) comparison 
with the current typed text is moved up to just below the matchCompletion 
comparison. This means that items with correctly matching case will match 
better than items with incorrect case (if they both start with the typed text), 
regardless of inheritance depth and alphabetical ordering.
  3. This comparison is also modified to only return true or false if only one 
of the items matches the typed text. The original code would yield inconsistent 
results if both items match the typed text, since the comparison would return 
true for both the test (a < b) and the test (b < a).
  4. When determining whether a completion item matches the typed text, the 
diff makes matchesAbbreviation() take model->matchCaseSensitivity() into 
account (as the other match tests already do). This fixes the issue that 
without this change, "test" and "TEST" would be considered to match exactly 
with model->matchCaseSensitivity() == Qt::CaseSensitive, since the abbreviation 
matching would treat it as a match.
  
  Regarding point 2, this change will only sort those completion items by 
case-compatibility that start with the typed text, but not the others. An 
alternative would be to determine case-compatibility of the item with the typed 
text when matching (in KateCompletionModel::Item::match()). Then completion 
items with the same MatchType could be sorted by case-compatibility (either in 
binary form, "matches case" vs. "does not match case", or by counting the 
number of letters with differing case).

TEST PLAN
  completion_test still passes. Added four checks to this test which test 
case-sensitive matching with matchesAbbreviation(). Did some manual testing.

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

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

D17904: Highlight CUDA .cu and .cuh files as C++

[Thomas Schöps]

thomassc updated this revision to Diff 48519.
thomassc added a comment.


  Thanks for having a look. I assume that the "version" attribute needs to be 
increased and the "kateversion" does not. I updated the diff accordingly. I 
don't think that I have any commit rights, can you commit?

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17904?vs=48493=48519

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

AFFECTED FILES
  data/syntax/cpp.xml

To: thomassc, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, bmortimer, hase, michaelh, 
genethomas, ngraham, bruns, demsking, cullmann, vkrause, sars

D17904: Highlight CUDA .cu and .cuh files as C++

[Thomas Schöps]

thomassc created this revision.
thomassc added a reviewer: Framework: Syntax Highlighting.
thomassc added a project: Framework: Syntax Highlighting.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
thomassc requested review of this revision.

REVISION SUMMARY
  This patch makes the syntax highlighting use the C++ style for CUDA sources 
(.cu) and headers (.cuh) by default. This should work very well, since those 
are essentially C++ files with very little additional syntax.

TEST PLAN
  Shortly tested manually in KDevelop, looked good.

REPOSITORY
  R216 Syntax Highlighting

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

AFFECTED FILES
  data/syntax/cpp.xml

To: thomassc, #framework_syntax_highlighting
Cc: kwrite-devel, kde-frameworks-devel, bmortimer, hase, michaelh, genethomas, 
ngraham, bruns, demsking, cullmann, vkrause, sars, dhaumann