Differential D6398

Fix highlighting when editing in patch review
ClosedPublic
Actions

Authored by croick on Jun 26 2017, 11:08 PM.

Details

Reviewers
apol
Group Reviewers
KDevelop
Commits
R33:e518fe2bfb9c: Fix highlighting when editing in patch review
R32:e518fe2bfb9c: Fix highlighting when editing in patch review
Summary

Problems:

  • removing markers in foreach loop lead to crash (BUG 380125)
  • highlighting when editing in patch review was broken
  • reloading would stop the highlighting

Fixes:

  • textInserted/textRemoved signals only send line-by-line (or character-by-character) changes and there are extra signals for newline insertion/removal. The patch reflects this mechanism.
  • Redo the highlighting when the document is reloaded.
  • Remove one of the duplicated lists of ranges (mainly to improve maintainability).

Persisting Issue:

  • possible dangling mark when removing the "wrong" newline from a set of newline-only-lines changes.

BUG: 380125

Test Plan
  • tested text insertion, text removal and mark clicking and the combination of those with (mostly) consistent results for a git diff

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
croick created this revision.Jun 26 2017, 11:08 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 26 2017, 11:08 PM
apol accepted this revision.Jun 28 2017, 9:06 AM
This revision is now accepted and ready to land.Jun 28 2017, 9:06 AM
Closed by commit R33:e518fe2bfb9c: Fix highlighting when editing in patch review (authored by croick). · Explain WhyJun 28 2017, 10:38 PM
This revision was automatically updated to reflect the committed changes.
rjvbb added a subscriber: rjvbb.Jul 3 2017, 12:10 PM

This patch needs a bit of refactoring to make it apply to the 5.1 branch. It builds and seems to run, but is it safe to cherrypick (as in no hidden dependencies on other changes that would require merging too)?

apol added a comment.Jul 3 2017, 12:39 PM

I don't think it would be a good idea to get this big patch in 5.1.

kfunk added a subscriber: kfunk.Jul 4 2017, 6:14 AM
In D6398#121295, @apol wrote:

I don't think it would be a good idea to get this big patch in 5.1.

+1

rjvbb added a comment.Jul 4 2017, 8:55 AM

That doesn't really answer my question...
So is there another simpler way to fix the confirmed bug it addresses, regardless of whether it is committed to the 5.1 branch or applied as a packaging/distribution patch?

Revision Contents
Changeset List

Diff 15973

View Options

plugins/patchreview/patchhighlighter.h

Loading...
View Options

plugins/patchreview/patchhighlighter.cpp

Loading...