Review KateGotoBar
ClosedPublic

Authored by loh.tar on Thu, Jan 10, 6:52 PM.

Details

Reviewers
cullmann
Group Reviewers
KTextEditor
Commits
R39:3b806f3b40f7: Review KateGotoBar
Summary
  • Set minimum value of spin-box to 1
  • Update spinbox boundaries in rare cases where the bar is visible while document is edited
  • 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

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
loh.tar created this revision.Thu, Jan 10, 6:52 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptThu, Jan 10, 6:52 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Thu, Jan 10, 6:52 PM
anthonyfieroni added inline comments.
src/dialogs/katedialogs.cpp
1130

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

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.

src/dialogs/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?

anthonyfieroni added inline comments.Fri, Jan 11, 6:49 AM
src/dialogs/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.

cullmann added inline comments.
src/dialogs/katedialogs.cpp
1130

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.

loh.tar updated this revision to Diff 49243.Fri, Jan 11, 2:42 PM
  • 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.

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 think <CTRL+G> <CTRL+V> <ENTER> is quite fast. In addition, this leads to having "Gehe zu" twice in the ui, which is a bit confusing imho. Other opinions?

src/dialogs/katedialogs.cpp
1099

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

1156

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

src/dialogs/katedialogs.h
107

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

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 think <CTRL+G> <CTRL+V> <ENTER> is 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"

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

cullmann accepted this revision.Wed, Jan 16, 8:18 AM

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!

This revision is now accepted and ready to land.Wed, Jan 16, 8:18 AM
loh.tar updated this revision to Diff 49636.Wed, Jan 16, 2:21 PM
  • 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 without success. 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
  • 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 without success. 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.

loh.tar updated this revision to Diff 49653.Wed, Jan 16, 4:32 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • Morph updateData() into a slot
  • connect to Document::textChanged signal

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.

loh.tar updated this revision to Diff 49665.Wed, Jan 16, 8:00 PM
  • 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.

cullmann accepted this revision.Thu, Jan 17, 12:03 PM

Yep ;=)

Thanks for the improvements, will merge.

This revision was automatically updated to reflect the committed changes.