D18164: Review KateGotoBar

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=49708 REVISION

D18164: Review KateGotoBar

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,

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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,

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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,

D18164: Review KateGotoBar

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,

D18164: Review KateGotoBar

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 ==

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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

D18164: Review KateGotoBar

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