Re: Review Request: Add keyboard shortcut for collection search

[Silver Juurik]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102956/
---

(Updated Oct. 24, 2011, 5:55 p.m.)


Review request for Amarok.


Changes
---

When pressing ctrl+F, the collection browser is now opened. 

amarok://navigate/collections without any filter parameters seemed to do the 
trick.


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 (updated)
-

  src/browsers/collectionbrowser/CollectionWidget.h a206914 
  src/browsers/collectionbrowser/CollectionWidget.cpp ace058a 
  src/MainWindow.h 076628f 
  src/MainWindow.cpp 2d2ebac 

Diff: http://git.reviewboard.kde.org/r/102956/diff/diff


Testing
---


Thanks,

Silver Juurik

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: Add keyboard shortcut for collection search

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102956/#review7587
---

Ship it!


Done, commited as 
http://commits.kde.org/amarok/b4b59198a3b991df198a294f7abb1e2f6d945e56 (whith 
some slight modifications)

Thanks for making Amarok better. Silver, plese mark this request as submitted, 
I don't (yet) have permissions to do so.

- Matěj Laitl


On Oct. 24, 2011, 5:55 p.m., Silver Juurik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102956/
> ---
> 
> (Updated Oct. 24, 2011, 5:55 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/browsers/collectionbrowser/CollectionWidget.h a206914 
>   src/browsers/collectionbrowser/CollectionWidget.cpp ace058a 
>   src/MainWindow.h 076628f 
>   src/MainWindow.cpp 2d2ebac 
> 
> Diff: http://git.reviewboard.kde.org/r/102956/diff/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silver Juurik
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: Add keyboard shortcut for collection search

[Daniel Faust]

> On Oct. 24, 2011, 4:56 p.m., Matěj Laitl wrote:
> > Good, thanks for your work, Silver.
> > 
> > I've found an interesting corner case - while the keyboard is always 
> > focused to collection search upon CTRL+F, the collection widget itself does 
> > not show itself if it was previously hidden - see these screenshots to see 
> > when it happens: http://www.laitl.cz/soubory/amarok-ctrl-f-1.jpeg 
> > http://www.laitl.cz/soubory/amarok-ctrl-f-2.jpeg
> > 
> > You may perhaps have luck looking at the "Amarok bookmarks" code to see how 
> > to raise Collection browser widget - especially the part dealing with 
> > amarok://navigate/... urls.

Looks like a very nice feature.
And as somebody having tabbed the collection view myself, I would like to see 
the calling of a bookmark, too.

Something like this should do the trick.

#include "amarokurls/AmarokUrl.h"

AmarokUrl bookmark( "amarok://navigate/collections" );
bookmark.run();


- Daniel


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102956/#review7581
---


On Oct. 24, 2011, 4:31 p.m., Silver Juurik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102956/
> ---
> 
> (Updated Oct. 24, 2011, 4:31 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
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: Add keyboard shortcut for collection search

[Silver Juurik]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102956/
---

(Updated Oct. 24, 2011, 4:31 p.m.)


Review request for Amarok.


Changes
---

Renamed the methods to slotFocusPlaylistSearch and slotFocusCollectionSearch. 
The text for the "Jump to" action changed to "Search playlist". Debug blocks 
removed.


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 (updated)
-

  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
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: Add keyboard shortcut for collection search

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102956/#review7581
---


Good, thanks for your work, Silver.

I've found an interesting corner case - while the keyboard is always focused to 
collection search upon CTRL+F, the collection widget itself does not show 
itself if it was previously hidden - see these screenshots to see when it 
happens: http://www.laitl.cz/soubory/amarok-ctrl-f-1.jpeg 
http://www.laitl.cz/soubory/amarok-ctrl-f-2.jpeg

You may perhaps have luck looking at the "Amarok bookmarks" code to see how to 
raise Collection browser widget - especially the part dealing with 
amarok://navigate/... urls.

- Matěj Laitl


On Oct. 24, 2011, 4:31 p.m., Silver Juurik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102956/
> ---
> 
> (Updated Oct. 24, 2011, 4:31 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
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: Add keyboard shortcut for collection search

[Matěj Laitl]

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


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


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
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel