D18164: Review KateGotoBar

[Christoph Cullmann]

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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18164?vs=49665&id=49708

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h

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

D18164: Review KateGotoBar

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  Yep ;=)
  
  Thanks for the improvements, will merge.

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

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

D18164: Review KateGotoBar

[loh tar]

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


  - Connect/Disconnect
  - Remove m_gotoRange->setFocus
  
  Like this? :-/
  
  I noticed bad behavior and have remove these setFocus, test it if its OK.
  On edit was always the focus set to the spinbox.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18164?vs=49653&id=49665

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h

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

D18164: Review KateGotoBar

[Christoph Cullmann]

cullmann added a comment.


  I would disconnect the textChanged in ::closed() to avoid having updates 
there even if the bar is hidden again (and reconnect on show).
  
  Otherwise I think this is ready to go in.

REPOSITORY
  R39 KTextEditor

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

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

D18164: Review KateGotoBar

[loh tar]

loh.tar updated this revision to Diff 49653.
loh.tar edited the summary of this revision.
loh.tar set the repository for this revision to R39 KTextEditor.
loh.tar added a comment.


  - Morph updateData() into a slot
  - connect to Document::textChanged signal

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18164?vs=49636&id=49653

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h

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

D18164: Review KateGotoBar

[Christoph Cullmann]

cullmann added a comment.


  In D18164#394498 , @loh.tar wrote:
  
  > - Fix typo of member vars
  > - Add missing setView(m_view)
  > - Remove FIXME hint about timer, however would a comment for me nice
  > - Use QString()
  >
  >   Things which could be improved or not
  > - The up/down buttons are a little bit small, have tried to give them the 
hight (and width) of the hight of the other buttons, but withoutsuccess. 
Ideas?
  > - There is no tooltip on the spinbox and Goto button
  
  
  I can live with that ;=)
  
  > - The spinbox will not adjust its size and value boundaries when you paste 
text while its shown. Try:
  >   - New file->show bar
  >   - Paste 10+ lines
  >   - Spinbox keep size of one digit and can't be set to an other value as 1
  
  Perhaps one wants to connect to the textChanged signal and update the range.

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

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

D18164: Review KateGotoBar

[loh tar]

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


  - Fix typo of member vars
  - Add missing setView(m_view)
  - Remove FIXME hint about timer, however would a comment for me nice
  - Use QString()
  
  Things which could be improved or not
  
  - The up/down buttons are a little bit small, have tried to give them the 
hight (and width) of the hight of the other buttons, but withoutsuccess. 
Ideas?
  - There is no tooltip on the spinbox and Goto button
  - The spinbox will not adjust its size and value boundaries when you paste 
text while its shown. Try:
- New file->show bar
- Paste 10+ lines
- Spinbox keep size of one digit and can't be set to an other value as 1

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18164?vs=49243&id=49636

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h

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

D18164: Review KateGotoBar

[Christoph Cullmann]

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


  I think it is a nice enhancement over the current state.
  
  I give it an accept, but wait for the "Regarding code comments, will fix it 
when it got in general an "OK"" before I apply it, thanks!

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

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

D18164: Review KateGotoBar

[Dominik Haumann]

dhaumann added a comment.


  Indeed I misread the screenshot - sorry for that :-) In that case, I have no 
objections. @cullmann your take?

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

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

D18164: Review KateGotoBar

[loh tar]

loh.tar added a comment.


  > I have the feeling it adds more clutter than it helps by default:
  
  Code clutter or what? The functionality is lovely and I can't see in which 
way It may someone jar
  
  > I thinkis quite fast.
  
  For a keyboard-virtuoso for sure. I'm one of those who use often the mouse.
  
  > this leads to having "Gehe zu" twice in the ui, which is a bit confusing 
imho. Other opinions?
  
  Um, I think you misread my post. That's my comment to the current situation, 
without this patch(?) With patch is the pic in the test plan.
  
  Regarding code comments, will fix it when it got in general an "OK"

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

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

D18164: Review KateGotoBar

[Dominik Haumann]

dhaumann added a comment.


  I like the idea to go to next modified line up / down. I am undecided about 
the goto line in clipboard, since I have the feeling it adds more clutter than 
it helps by default: I thinkis quite fast. In 
addition, this leads to having "Gehe zu" twice in the ui, which is a bit 
confusing imho. Other opinions?

INLINE COMMENTS

> katedialogs.cpp:1099
> +btn->setIcon(QIcon::fromTheme(QStringLiteral("go-up-search")));
> +btn->setText(QStringLiteral(""));
> +btn->installEventFilter(this);

Please always use QString() in favor of QStringLiteral("")

> katedialogs.cpp:1156
> +message->setAutoHide(2000); // FIXME Timer is not started as I would 
> expect, only after some "unclear" event
> +message->setPosition(KTextEditor::Message::BottomInView);
> +m_view->document()->postMessage(message);

You should also call message->setView(m_view), otherwise the message appears in 
all views, in case you have multiple views of a document.

> katedialogs.h:103
> +QSpinBox *m_gotoRange = nullptr;
> +QToolButton *m_ModifiedUp = nullptr;
> +QToolButton *m_ModifiedDown = nullptr;

Could you use lowercase naming here? m_modifiedUp instead of m_ModifiedUp?

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

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

D18164: Review KateGotoBar

[loh tar]

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


  - Use 120 as wheel-delta threshold
  - Use member instead of static local
  
  > If not do it right you can end up in partial value when it used 
finer-resolution wheels and mishmash.
  
  I have problems to see that hazard, na matter.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18164?vs=49185&id=49243

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h

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

D18164: Review KateGotoBar

[Christoph Cullmann]

cullmann added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in katedialogs.cpp:1130
> > you can either cumulatively add the delta values from events until the 
> > value of 120 is reached
> 
> I don't know what is unclear, it should be 120 not any other value.
> 
>   if (object == m_ModifiedUp) {
>   m_deltaUp += event->delta();
>   if (m_deltaUp >= 120) {
>   m_ModifiedUp->click();
>   }
> 
> and so on. If not do it right you can end up in partial value when it used 
> finer-resolution wheels and mishmash.

I think 120 would be better to use, as that is the value one should start to 
handle given the docs.
I would not use a static delta but just have the delta as member of the class.

REPOSITORY
  R39 KTextEditor

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

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

D18164: Review KateGotoBar

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> loh.tar wrote in katedialogs.cpp:1130
> > It should be 120,
> 
> Why? Have now read the doc, but without a new insight.
> 
> > also you can have 2 separate delta members
> 
> How and why?

> you can either cumulatively add the delta values from events until the value 
> of 120 is reached

I don't know what is unclear, it should be 120 not any other value.

  if (object == m_ModifiedUp) {
  m_deltaUp += event->delta();
  if (m_deltaUp >= 120) {
  m_ModifiedUp->click();
  }

and so on. If not do it right you can end up in partial value when it used 
finer-resolution wheels and mishmash.

REPOSITORY
  R39 KTextEditor

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

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

D18164: Review KateGotoBar

[loh tar]

loh.tar added a comment.


  I was about to move StatusBarButton into kateviewhelpers, so that this button 
can used elsewhere too, like here. But got stuck. However, I think such button 
would be handy. Perhaps also a  KateViewBarLayout. 
  Here shot regarding 'Change label text to be less redundant in conjunction 
with the "goto-button"' At least in German is that old text terrible.
  F6539614: 1547187451.png 

INLINE COMMENTS

> anthonyfieroni wrote in katedialogs.cpp:1130
> https://doc.qt.io/qt-5/qwheelevent.html#angleDelta
> It should be 120, also you can have 2 separate delta members

> It should be 120,

Why? Have now read the doc, but without a new insight.

> also you can have 2 separate delta members

How and why?

REPOSITORY
  R39 KTextEditor

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

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

D18164: Review KateGotoBar

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> katedialogs.cpp:1130
> +// and with my mousepad was the experience also flawlessly
> +if (delta > 100) {
> +delta = 0;

https://doc.qt.io/qt-5/qwheelevent.html#angleDelta
It should be 120, also you can have 2 separate delta members

REPOSITORY
  R39 KTextEditor

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

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

D18164: Review KateGotoBar

[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
  - Set minimum value of spin-box to 1
  - Don't force minimum width by buttons, see similar Bug 402904
  - Rename gotoRange -> m_gotoRange
  - Add modified_line_up/down buttons with mouse wheel support
  - Change label text to be less redundant in conjunction with the "goto-button"
  - Change QLabel to button, with an action to go to line number from 
clipboard. My first intend was to add a clear-button to the spin box (not so 
easy) because paste from clipboard needs an empty field. But then I had this 
idea which is much more handy.

TEST PLAN
  F6539214: 1547145791.png 

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h

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