----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102956/#review7573 -----------------------------------------------------------
Looks okay to me, just a few minor things (see below). Silver, would you also mind correcting the "Jump to" action and methods a bit? I think both user-visible KAction name "Jump to" and method name slotJumpTo are uninformative, it should be something more specific such as "Jump to playlist". (or "Search playlist" to be on par with the greyed out text in the input button and "Search collection") If you want, add this to your patch. (you may also remove the DEBUG_BLOCK) src/MainWindow.h <http://git.reviewboard.kde.org/r/102956/#comment6565> This perhaps deserves a doc comment that states it just focuses collection search bar, otherwise the name is a bit misleading. (it doesn't search anything it just focuses the bar) src/MainWindow.cpp <http://git.reviewboard.kde.org/r/102956/#comment6566> Is the DEBUG_BLOCK necessary here? I think it would just pollute debug log. - Matěj Laitl On Oct. 23, 2011, 8:17 p.m., Silver Juurik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102956/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2011, 8:17 p.m.) > > > Review request for Amarok. > > > Description > ------- > > The patch adds a keyboard shortcut for moving focus to collection search > input box. The default shortcut is ctrl+F. > > The idea is from a JJ: https://bugs.kde.org/show_bug.cgi?id=257381 > The keyboard shortcuts ctrl+J (an existing shortcut that moves focus to > playlist search box) and ctrl+F partially solve the problem described in the > bug. > > > Diffs > ----- > > src/MainWindow.h 076628f > src/MainWindow.cpp 2d2ebac > src/browsers/collectionbrowser/CollectionWidget.h a206914 > src/browsers/collectionbrowser/CollectionWidget.cpp ace058a > > Diff: http://git.reviewboard.kde.org/r/102956/diff/diff > > > Testing > ------- > > > Thanks, > > Silver Juurik > >
_______________________________________________ Amarok-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/amarok-devel
