D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  > I like the new status bar.
  
  Thanks :-)
  
  > In "modified" state it would be cool to be able to click to save the 
document
  
  Yes, that was also my idea. IIRC I had somewhere asked but got no response. 
Will do it soon.
  
  > Move the modified label from far right to far left
  
  Sadly is now the Close button e.g. of the goto bar at the same position. So 
closing the goto bar may then unintended save the document when you have some 
mouse trouble
  
  > In "unedited" state the purpose is (still) hard to guess from the icon (and 
there is the convention of not having tooltips for the status bar which 
requires a creative solution).
  
  I agree, I'm also not so happy with that. My idea was to show a tool-tip on 
click. Sadly is that not a "one-liner" in Qt, need some quirks.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[gregormi]

gregormi added a comment.


  @loh.tar: I like the new status bar.
  
  > Move the modified label from far right to far left This way have it a more 
prominent position
  
  Now, the "modified label" behaves like a button but with no visible action 
when it is clicked. Two general observations:
  
  - In "unedited" state the purpose is (still) hard to guess from the icon (and 
there is the convention of not having tooltips for the status bar which 
requires a creative solution).
  - In "modified" state it would be cool to be able to click to save the 
document

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:dce48b397818: Review KateStatusBar (authored by loh.tar, 
committed by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17730?vs=48813=49037

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

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

cullmann added a comment.


  Btw., I can resize both Kate and KWrite like I want, like in the screenshots 
visible above.

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  I think the current state can go in.
  
  Here with and without this patch two tests fail:
  
  The following tests FAILED:
  
24 - kateindenttest_testCppstyle (Failed)
67 - vimode_emulatedcommandbar (Failed)
  
  (perhaps due to Qt 5.12)

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  > It was stated that it broke tests, should this be addressed now?
  
  As shown in the pic, the bar forces no width anymore (at least it seems so)
  Your comments at the bug report are interesting, but I'm not sure to do it. I 
had Christoph promised to add options to customize what is shown, so that was 
on my mental list.

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

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

D17730: Review KateStatusBar

[Dominik Haumann]

dhaumann added a comment.


  Hm, what is the current state of this patch?
  
  It was stated that it broke tests, should this be addressed now?
  
  Btw, I added a detailed comment to 
https://bugs.kde.org/show_bug.cgi?id=402904 that explains a proper solution to 
shrinking the statusbar:
  
  1. by allowing compact notations (e.g. "Line 1, Column 1" would shrink to "L 
1, C 1")
  2. by allowing to hide widgets that are less important

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

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

D17730: Review KateStatusBar

[loh tar]

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


  - Remove not supported Rich Text from [BLOCK] hint
  - Add word count info to line label
  
  F6530066: pic.png 

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17730?vs=48781=48813

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

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

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

D17730: Review KateStatusBar

[loh tar]

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


  Allow StatusBarButton to shrink
  
  Ah! Strange! :-/ But somehow the more usual behavior, these "crunching" is 
unlikely. Couldn't find a hint where it is done in Kate, but after some digging 
around I have some solution which seems to work. These pics are from KWrite, 
the only not shrinking stuff is the last plain QLabel with the word count.
  F6529406: pic.png 
  I had didn't touch it in the past because I was thinking it may better to add 
these text to the line column label (now button), but that need some more 
thought how to do it.
  
  Let me know how to progress further, and if that test now pass.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17730?vs=48395=48781

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

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

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

D17730: Review KateStatusBar

[Francisco de Zuviría]

zetazeta added a comment.


  The problem I talk about happens on kwrite, no kate. I get the crunch as well 
on kate. Cheers :)

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  @zetazeta wrote
  
  > I find it annoying that the status bar imposes a limit on the minimum width 
of the window
  
  @dhaumann wrote
  
  > Maybe with this patch, the status bar grows and forces a different width
  
  me wrote
  
  > The bar can shrink almost endless
  
  Um...it is here as I said. I can remember to read @zetazeta post, but so far 
I remember was I a little bit confused about that statement. But who knows... A 
scroll able bar would had make sense anyway, too small buttons/labels are 
useless.
  
  I'm on X, no Wayland if that is somehow important, with no special stuff.
  
  Here the pic I had in mind to post earlier. 
  F6528279: pic.png 
  As you see some labels are crunched and the window width is the same. If you 
need I can post a pic where the limit is only given by the minimap and the line 
number column.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Francisco de Zuviría]

zetazeta added a comment.


  In D17730#382676 , @cullmann wrote:
  
  > > - Your comment to @zetazeta request ? I have tried to add a QScrollArea 
but without success :-/ Ideas? Perhaps is a second level of layout/widget 
needed(?)
  >
  > I would not take care of that now but think about that in a separate 
request.
  >  If we want that, perhaps one needs to think about having it scroll with 
the view, I don't think one wants a second scrollview.
  
  
  Howdy ho, great guys and gals.
  
  I have opened a wishlist for this in 
https://bugs.kde.org/show_bug.cgi?id=402904
  
  If you have discovered anything useful while reworking the status bar, please 
share it there, so life will be easier to the one that takes on the wish.
  
  Best regards and thanks for taking care of the status bar.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  That can I not really imagine :-/ Wouldn't that not somehow to be notice in 
normal use? 
  Let's try this. Shot without bar, Now choose "Show bar", The window didn't 
change. (Sorry, no tool at hand to paste pics together)
  The bar can shrink almost endless.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Dominik Haumann]

dhaumann added a comment.


  @loh.tar I think this test uses dynamic word wrap, and therefore requires a 
specific width of the view. Maybe with this patch, the status bar grows and 
forces a different width, so that the dynamic word wrap changes and the test 
fails?

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  > Do we loose this property by using real buttons instead of labels as this 
patch proposes?
  
  I don't think so. The buttons are not entire new, only some labels are now 
ALSO buttons.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Dominik Haumann]

dhaumann added a comment.


  @cullmann I remember that you fiddled around with the status bar quite a lot 
to make it pixel perfect in height, i.e. that the height of the statusbar 
matches the hight of e.g. the search bar. Do we loose this property by using 
real buttons instead of labels as this patch proposes?
  
  PS: I did not look at the patch in detail.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  Harr, one more issue :-/
  F6516314: 1546186710.png 
  
  Idea how to fix this? I had in the past often such trouble, but so far I 
remember no solution. So my fix would be to remove the emphasis.
  Is it OK to update this diff here?

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  One idea: Now you have only to click once to change some setting, not twice 
(double-click). So if these test simulate a clicks...

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar added a comment.


  I'm pretty baffled. What does the status bar which may break some input 
(mode) ? These report says nothing to me.
  Well, ok, the vi-mode can enabled/disabled :-/

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

cullmann added a comment.


  I reverted it ATM, to see if the tests pass then again.

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

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


  Hmm, this seems to break the unit tests :/
  
  see 
https://build.kde.org/job/Frameworks/job/ktexteditor/job/kf5-qt5%20SUSEQt5.9/167/testReport/junit/projectroot.autotests.src/vimode/vimode_view/
  
  >>> running command  "ljgjr."  on text  "stickyhelper\n 
 LINE3"
  
  QDEBUG : ViewTest::visualLineUpDownTests() org.kde.ktexteditor: Invalid 
position: ( 0 , -1 )
  FAIL!  : ViewTest::visualLineUpDownTests() Compared values are not the same
  
Actual   (kate_document->text()): "stickyhelper\n 
.XXX LINE3"
Expected (expected_text): "stickyhelper\n 
 .INE3"
Loc: [/home/jenkins/workspace/Frameworks/ktexteditor/kf5-qt5 
SUSEQt5.9/autotests/src/vimode/view.cpp(222)]
  
  QDEBUG : ViewTest::visualLineUpDownTests()
  
  >>> running command  "ljgjgkr."  on text  "stickyhelper\n 
 LINE3"
  
  QDEBUG : ViewTest::visualLineUpDownTests() org.kde.ktexteditor: Invalid 
position: ( 0 , -1 )
  FAIL!  : ViewTest::visualLineUpDownTests() Compared values are not the same
  
Actual   (kate_document->text()): "stick.helper\n 
 LINE3"
Expected (expected_text): "stickyhelper\n 
.XXX LINE3"
Loc: [/home/jenkins/workspace/Frameworks/ktexteditor/kf5-qt5 
SUSEQt5.9/autotests/src/vimode/view.cpp(227)]
  
  QDEBUG : ViewTest::visualLineUpDownTests()

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:3b874e56b8dc: Review KateStatusBar (authored by loh.tar, 
committed by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17730?vs=48387=48395

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

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

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

D17730: Review KateStatusBar

[Christoph Cullmann]

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


  Yep, fine with this.
  Thanks!

REPOSITORY
  R39 KTextEditor

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

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

D17730: Review KateStatusBar

[loh tar]

loh.tar updated this revision to Diff 48387.
loh.tar retitled this revision from "[WIP] Review KateStatusBar " to "Review 
KateStatusBar ".
loh.tar edited the summary of this revision.
loh.tar added a comment.


  - Remove seemingly unneeded spacing. Well, there is a change but tiny and 
perhaps to the better
  - Avoid ugly casting in custom menu
  - Remove obviously unneded default stretch values
  - Avoid obviously unneded KLocalizedString members
  - Avoid Q_FOREACH
  - Group some lines more logical
  
  I hope you still like it. Dictionary button will come in extra patch.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17730?vs=48374=48387

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

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

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