D20697: Review IconBorder

[loh tar]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:0d70744ed67b: Fix broken InlineNoteTest (authored by 
loh.tar).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D20697?vs=56843=56895#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20697?vs=56843=56895

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

AFFECTED FILES
  autotests/src/inlinenote_test.cpp

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

D20697: Review IconBorder

[Christoph Cullmann]

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


  loh.tar, can you take a look?
  If it is too complex to fix easily, we can still revert this and apply it 
again later together with a fix.
  Thanks for pointing the CI fail out David!

REPOSITORY
  R39 KTextEditor

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

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

D20697: Review IconBorder

[Christoph Cullmann]

cullmann added a comment.


  :( Sorry, I didn't run them again, just tried out if it works in KDevelop.

REPOSITORY
  R39 KTextEditor

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

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

D20697: Review IconBorder

[David Faure]

dfaure added a comment.


  This commit appears to have introduced a unittest regression
  
  FAIL!  : InlineNoteTest::testInlineNote() Compared values are not the same
  
Actual   (newCoordCol04): QPoint(51,1)
Expected (coordCol04)   : QPoint(33,1)
Loc: [/home/jenkins/workspace/Frameworks/ktexteditor/kf5-qt5 
SUSEQt5.10/autotests/src/inlinenote_test.cpp(171)]
  
  
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.10/job/ktexteditor/job/kf5-qt5%20SUSEQt5.10/98/testReport/junit/projectroot/autotests/inlinenote_test/

REPOSITORY
  R39 KTextEditor

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

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

D20697: Review IconBorder

[loh tar]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:57781f34d234: Review IconBorder (authored by loh.tar).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20697?vs=56839=56843

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

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

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

D20697: Review IconBorder

[Christoph Cullmann]

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


  This works for me fine with KDevelop.
  Please commit.
  Thanks

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

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

D20697: Review IconBorder

[loh tar]

loh.tar updated this revision to Diff 56839.
loh.tar edited the test plan for this revision.
loh.tar added a comment.


  try to fix annotation issue
  
  completely untested :-/

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20697?vs=56694=56839

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

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

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

D20697: Review IconBorder

[Christoph Cullmann]

cullmann added a comment.


  I am not sure one can delay the updateGeometry stuff until one paints.

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

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

D20697: Review IconBorder

[Christoph Cullmann]

cullmann requested changes to this revision.
cullmann added a comment.
This revision now requires changes to proceed.


  Unfortunately, the annotation stuff regressed.
  I tried KDevelop, right click on text => Git -> Annotation...
  See pre-patch and post-patch pictures below, the too small one is the post 
patch one.
  F6788986: pre-patch.png 
  F6788988: with-patch.png 

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

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

D20697: Review IconBorder

[Dominik Haumann]

dhaumann added a comment.


  Well, issues that were already there before should not hinder this patch of 
course! If you think this is good enough, please go on.

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

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

D20697: Review IconBorder

[loh tar]

loh.tar added a comment.


  In D20697#453841 , @dhaumann wrote:
  
  > Did you test that the annotation border still works? You can do so in 
KDevelop by invoking git blame.
  
  
  :-/ ...OK, thanks
  
  > Besides I still gave no issues with this patch, except that I did not test 
myself. If you say there are mouse move issues, then these issues should be 
fixed asap :)
  
  It's no new issue..so it's not so urgent  :P

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

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

D20697: Review IconBorder

[Dominik Haumann]

dhaumann added a comment.


  Did you test that the annotation border still works? You can do so in 
KDevelop by invoking git blame.
  
  Besides I still gave no issues with this patch, except that I did not test 
myself. If you say there are mouse move issues, then these issues should be 
fixed asap :)

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

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

D20697: Review IconBorder

[loh tar]

loh.tar added a comment.


  @dhaumann Beside these annotation stuff I think I'm done with this now. 
Should you, or someone else, not stop me in the next few days I may treat this 
as OK and push it.
  There are still some issues in the mouse move handling but will try to fix 
that in some other patch

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

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

D20697: Review IconBorder

[loh tar]

loh.tar edited the test plan for this revision.

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

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

D20697: Review IconBorder

[loh tar]

loh.tar updated this revision to Diff 56694.
loh.tar edited the summary of this revision.
loh.tar added a comment.


  - Fix bookmark pixmap painting
  
  Left before, right with fix
  F6786476: bookmark-pixmap.png 

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20697?vs=56693=56694

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

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

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

D20697: Review IconBorder

[loh tar]

loh.tar updated this revision to Diff 56693.
loh.tar edited the summary of this revision.
loh.tar added a comment.


  - Add margin to the edit area, but I'm not sure if it should be done here. I 
guess renderer should do it
  - Simplify "additional folding highlighting", there is now the slightly 
gradient gone, bad?
  - Rename backGroundColor->iconBarColor to fit orig name, but that name sounds 
to me as it's only for the icon area of the border
  
  F6786421: pic.png 

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20697?vs=56644=56693

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

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

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

D20697: Review IconBorder

[loh tar]

loh.tar updated this revision to Diff 56644.
loh.tar added a comment.


  - Fix missing printed background in proper theme color
  - Fix scroll past end of document
  - Fix less pushy paint unfolded icon in not dark themes and don't try to use 
currentLineNumberColor, the folded icon gets also not highligted
  - Some more cosmetic
  
  F6785125: pic.png 

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20697?vs=56625=56644

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

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

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

D20697: Review IconBorder

[Dominik Haumann]

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


  I like the visual change, let's give it a try. Thanks!

REPOSITORY
  R39 KTextEditor

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

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

D20697: Review IconBorder

[loh tar]

loh.tar edited the summary of this revision.
loh.tar edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

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

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

D20697: Review IconBorder

[loh tar]

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

REVISION SUMMARY
  The only two included changes noticeable by the user are:
  

  
  - Use line number color for unfolded triangle to be less intrusive
  - Move the line modification hint away from the edit area between the line 
numbers and the folding area to be less irritating when cursor is in first 
column
  
  Most of the changes in this patch could you see as code cosmetic or an 
attempt to make it more maintenance-friendly.
  

  
  - Cleanup, avoid redundancies and nesting
  - Rename members, make them unified named
  - Give each area an own value name
  - Remove m_oldBackgroundColor, can't see any benefit
  - Make positionToArea() and sizeHint() less error prone in case of design 
changes

TEST PLAN
  Pls try it, but I'm not sure if I'm done with it
  
  F6783737: 1555761488.png 

REPOSITORY
  R39 KTextEditor

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

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

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