Re: Review Request: Fix for USB storage mode media players

[Matěj Laitl]

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



src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp


This would break identification of iPods. KDE's solid with udisks & udev 
backend currently doesn't attach PMP interface to them.


- Matěj Laitl


On Dec. 8, 2010, 9:55 p.m., Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated Dec. 8, 2010, 9:55 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš Tinkl
> 
>

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

Re: Review Request: Fix for USB storage mode media players

[Ralf Engels]

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


Just noticed that the patch does not apply any longer.
Do you want to fix it yourself?

- Ralf


On Dec. 8, 2010, 9:55 p.m., Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated Dec. 8, 2010, 9:55 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš
> 
>

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

Re: Review Request: Fix for USB storage mode media players

[Ralf Engels]

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

Ship it!


Apart from the small thing with the drive == 0 everything seems to be quite 
good.
I would ship it with the additional check.

- Ralf


On Dec. 8, 2010, 9:55 p.m., Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated Dec. 8, 2010, 9:55 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš
> 
>

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

Re: Review Request: Fix for USB storage mode media players

[Ralf Engels]

> On Dec. 27, 2010, 9:19 a.m., Sergey Ivanov wrote:
> > src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp, 
> > line 74
> > 
> >
> > We still need check drive for existence to prevent crashes. What was 
> > wrong with HP devices?

I agree. The drive check should probably still be there.


- Ralf


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


On Dec. 8, 2010, 9:55 p.m., Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated Dec. 8, 2010, 9:55 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš
> 
>

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

Re: Review Request: Fix for USB storage mode media players

[Mark Kretschmann]

> On 2010-12-27 09:19:36, Sergey Ivanov wrote:
> > src/MediaDeviceCache.cpp, line 89
> > 
> >
> > & has greater priority then &&, parenthesis don't make difference.
> 
> Stefan Derkits wrote:
> Parenthesis make no difference for the compiler (and most probably also 
> no Speed Difference), but in cases where the Operator Precendence is not that 
> well known, they make the code easier to read --> I would leave them

I agree with Stefan. Better readability is a good thing.


- Mark


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


On 2010-12-08 21:55:12, Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated 2010-12-08 21:55:12)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš
> 
>

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

Re: Review Request: Fix for USB storage mode media players

[Stefan Derkits]

> On 2010-12-27 09:19:36, Sergey Ivanov wrote:
> > src/MediaDeviceCache.cpp, line 89
> > 
> >
> > & has greater priority then &&, parenthesis don't make difference.

Parenthesis make no difference for the compiler (and most probably also no 
Speed Difference), but in cases where the Operator Precendence is not that well 
known, they make the code easier to read --> I would leave them


- Stefan


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


On 2010-12-08 21:55:12, Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated 2010-12-08 21:55:12)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš
> 
>

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

Re: Review Request: Fix for USB storage mode media players

[Sergey Ivanov]

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



src/MediaDeviceCache.cpp


& has greater priority then &&, parenthesis don't make difference.



src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp


We still need check drive for existence to prevent crashes. What was wrong 
with HP devices?


- Sergey


On 2010-12-08 21:55:12, Lukáš Tinkl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100194/
> ---
> 
> (Updated 2010-12-08 21:55:12)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> ---
> 
> This patch fixes identifying general USB storage mode media players, plus 
> adds some minor fixes and cleanups. The main change is in 
> UmsConnectionAssistant::identify method.
> 
> 
> Diffs
> -
> 
>   src/MediaDeviceCache.cpp babb8ff 
>   
> src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 
> 92339ff 
>   src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 
> 5956a2b 
> 
> Diff: http://git.reviewboard.kde.org/r/100194/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Amarok in USB mode
>   http://git.reviewboard.kde.org/r/100194/s/21/
> 
> 
> Thanks,
> 
> Lukáš
> 
>

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