Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-30 Thread Commit Hook 

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


This review has been submitted with commit 
684a218b3e223a3189b7290df90e6b22f52a514a by Mark Kretschmann on behalf of 
fleissig fleissig to branch master.

- Commit Hook


On June 28, 2013, 8:57 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 8:57 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-30 Thread Commit Hook 

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

(Updated June 30, 2013, 11:54 a.m.)


Status
--

This change has been marked as submitted.


Review request for Amarok.


Description
---

putting Artist - Title of the current track to the clipboard using a shortcut


This addresses bug 228872.
https://bugs.kde.org/show_bug.cgi?id=228872


Diffs
-

  src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
  src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 

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


Testing
---


Thanks,

fleissig fleissig

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-30 Thread Matěj Laitl 


> On June 28, 2013, 6:59 p.m., Mark Kretschmann wrote:
> > Nice patch! I recall we used to have this feature in an earlier Amarok 
> > version, but it got lost at some point :)
> > 
> > Do you have push rights for kde git, or should we push it for you?
> 
> fleissig fleissig wrote:
> I don't have rights, you should push it.
> 
> Mark Kretschmann wrote:
> Do you want "fleissig fleissig" as your name in the ChangeLog, or do you 
> prefer another name?

I'd even go further, we do really want to have contributions under the real 
names, be it for proper attribution or copyright matters.


- Matěj


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


On June 28, 2013, 10:57 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 10:57 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-30 Thread Mark Kretschmann 


> On June 28, 2013, 4:59 p.m., Mark Kretschmann wrote:
> > Nice patch! I recall we used to have this feature in an earlier Amarok 
> > version, but it got lost at some point :)
> > 
> > Do you have push rights for kde git, or should we push it for you?
> 
> fleissig fleissig wrote:
> I don't have rights, you should push it.

Do you want "fleissig fleissig" as your name in the ChangeLog, or do you prefer 
another name?


- Mark


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


On June 28, 2013, 8:57 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 8:57 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread fleissig fleissig 


> On June 28, 2013, 7:39 p.m., Matěj Laitl wrote:
> > src/MainWindow.cpp, lines 725-738
> > 
> >
> > Oh this is convoluted. What about
> > 
> > QString text;
> > Meta::ArtistPtr artist = currentTrack->artist();
> > if( artist )
> > text = artist->prettyName() + " - ";
> > text += currentTrack->prettyName()
> > 
> > clipboard->setText( text );

ok


- fleissig


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


On June 28, 2013, 8:57 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 8:57 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread fleissig fleissig 

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

(Updated June 28, 2013, 8:57 p.m.)


Review request for Amarok.


Description
---

putting Artist - Title of the current track to the clipboard using a shortcut


This addresses bug 228872.
https://bugs.kde.org/show_bug.cgi?id=228872


Diffs (updated)
-

  src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
  src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 

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


Testing
---


Thanks,

fleissig fleissig

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread Matěj Laitl 

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



src/MainWindow.cpp


Oh this is convoluted. What about

QString text;
Meta::ArtistPtr artist = currentTrack->artist();
if( artist )
text = artist->prettyName() + " - ";
text += currentTrack->prettyName()

clipboard->setText( text );


- Matěj Laitl


On June 28, 2013, 9:18 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 9:18 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread fleissig fleissig 

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

(Updated June 28, 2013, 7:18 p.m.)


Review request for Amarok.


Description
---

putting Artist - Title of the current track to the clipboard using a shortcut


This addresses bug 228872.
https://bugs.kde.org/show_bug.cgi?id=228872


Diffs (updated)
-

  src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
  src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 

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


Testing
---


Thanks,

fleissig fleissig

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread fleissig fleissig 


> On June 28, 2013, 4:59 p.m., Mark Kretschmann wrote:
> > Nice patch! I recall we used to have this feature in an earlier Amarok 
> > version, but it got lost at some point :)
> > 
> > Do you have push rights for kde git, or should we push it for you?

I don't have rights, you should push it.


- fleissig


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


On June 28, 2013, 6:32 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 6:32 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread fleissig fleissig 


> On June 28, 2013, 5:05 p.m., Matěj Laitl wrote:
> > src/MainWindow.cpp, line 725
> > 
> >
> > I suggest safety against artist() being null. Note that:
> > if( ...->artist() )
> > ...->artist()->...
> > 
> > is unsafe, you need a temporary.

I've added a check, but even a song with empty metadata had 
currentTrack->artist()->prettyName() as "Unknown Artist"


- fleissig


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


On June 28, 2013, 6:32 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 28, 2013, 6:32 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread fleissig fleissig 

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

(Updated June 28, 2013, 6:32 p.m.)


Review request for Amarok.


Description
---

putting Artist - Title of the current track to the clipboard using a shortcut


This addresses bug 228872.
https://bugs.kde.org/show_bug.cgi?id=228872


Diffs (updated)
-

  src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
  src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 

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


Testing
---


Thanks,

fleissig fleissig

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread Matěj Laitl 

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


Fine patch, I have just 2 minor suggestions.


src/MainWindow.cpp


I suggest safety against artist() being null. Note that:
if( ...->artist() )
...->artist()->...

is unsafe, you need a temporary.



src/MainWindow.cpp


nitpick: one extra whitespace


- Matěj Laitl


On June 27, 2013, 9:48 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 27, 2013, 9:48 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

2013-06-28 Thread Mark Kretschmann 

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

Ship it!


Nice patch! I recall we used to have this feature in an earlier Amarok version, 
but it got lost at some point :)

Do you have push rights for kde git, or should we push it for you?

- Mark Kretschmann


On June 27, 2013, 7:48 p.m., fleissig fleissig wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111275/
> ---
> 
> (Updated June 27, 2013, 7:48 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> putting Artist - Title of the current track to the clipboard using a shortcut
> 
> 
> This addresses bug 228872.
> https://bugs.kde.org/show_bug.cgi?id=228872
> 
> 
> Diffs
> -
> 
>   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
>   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
> 
> Diff: http://git.reviewboard.kde.org/r/111275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fleissig fleissig
> 
>

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