Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Commit Hook]

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


This review has been submitted with commit 
1a0287f7925d92a05a530ab83fffbc9afae67e86 by Matěj Laitl to branch master.

- Commit Hook


On March 16, 2012, 4:01 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
> ---
> 
> (Updated March 16, 2012, 4:01 p.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> ---
> 
> Rework transcoding: remember encoder, transcode on move, cleaner code
> 
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
> 
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
> 
> Why the hell the new Capability?
> 
> 
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferred transcoding
> paremeters etc.
> 
> I simply didn't want to pollute already over-crowded CollectionLocation
> with three more methods used by only a few subclasses. On the other
> hand, TranscodeCapability is not the central idea of this patch and I
> can factor it into CollectionLocation should there be a voice supporting
> it.
> 
> v2 patch version: gui string changes as suggested by Bart & Teo
> 
> [1] https://git.reviewboard.kde.org/r/103752/
> 
> FEATURE: 280526
> FEATURE: 264681
> CCBUG: 291722
> BUG: 263775
> FIXED-IN: 2.6
> REVIEW: 104213
> DIGEST: Feature: much improved transcoding
> 
> Transcoding::Property: remove NUMERIC, LIST, TEXT types
> 
> These types were not used since Teo reworked all encoders to use the
> TRADEOFF type. Remove them and associated code to make codebase cleaner
> so that new code doesn't need to introduce case statements in switches
> that will be never used, thus error-prone.
> 
> Individual types can be resurrected from this commit if there is a need
> for them in future.
> 
> CollectionLocation: display source dialog in next mainloop iteration
> 
> This is to make CollectionLocation::prepareCopy/Move() return fast as
> it advertises and not after several seconds when a modal dialog is
> shown.
> 
> 
> Diffs
> -
> 
>   ChangeLog bd777eadf39aec71efb74dfed7502f564553d998 
>   src/CMakeLists.txt

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Matěj Laitl]

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

(Updated March 16, 2012, 4:01 p.m.)


Review request for Amarok and Teo Mrnjavac.


Changes
---

gui string changes as suggested by Bart & Teo. I'll probably merge this later 
today.


Description (updated)
---

Rework transcoding: remember encoder, transcode on move, cleaner code

This is a major rework of transcoding feature that brings following
user-visible changes to Amarok:
 * Amarok can remember preferred transcoding configuration per each
   collection that supports transcoding. Therefore, the "Use default
   configuration" work-around can go away and the "Transcode or copy?"
   dialog can (and is) be one-step now. This preference can be changed
   in configuration.
 * Transcoding is now supported even during the move operation. No
   worries, only successfully transcoded tracks are removed from their
   original location.
 * Only formats playable on the target collection are offered. Already
   used & tested in yet-to-be-merged iPod collection rewrite.
 * The "Organize Tracks" dialog title and progress bar operation name
   now more verbosely describe actual operation to prevent user
   mistakes.
 * Double-transcode when ripping audio CDs that caused failures is
   avoided. (ChangeLog entry for this was miscredited to my earilier
   commit)

Technically, following changes are made:
 * many methods that accepted optional TranscodingConfiguration now
   either have it mandatory or not at all.
 * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
   INVALID along with convenience methods isValid() and isJustCopy().
   This simplifies logic in many methods.
 * CollectionLocation::prepare{Copy,Move}() now don't have optional
   TranscodingConfiguration parameter. Depending on target collection,
   CollectionLocation determines it automatically or asks user in
   showSourceDialog() (overridable). AudioCdCollectionLocation already
   overrides it.
 * Collections that support transcoding now should expose
   TranscodeCapability which is used to a) indicate that transcoding
   is supported; b) query which file formats are playable on target
   collection; c) read & save & unset preferred transcoding parameters.

Why the hell the new Capability?


Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
introduced the new one? In ideal world Amarok would be able to transcode
everything regardless of the target collection. This is however not
doable witch current copyUrlToCollection() design - target collection
needs to do non-trivial things such as re-reading file tags, accounting
for different file name and space requirements etc. See my comments in
[1]. We therefore need a way for target collection to indicate it
supports transcoding (in order not to fool user). Some collection
locations such as TrashCollectionLocation should even intentionally
disallow transcoding. Additionally, we want to be able to query
supported destination file formats, to save preferred transcoding
paremeters etc.

I simply didn't want to pollute already over-crowded CollectionLocation
with three more methods used by only a few subclasses. On the other
hand, TranscodeCapability is not the central idea of this patch and I
can factor it into CollectionLocation should there be a voice supporting
it.

v2 patch version: gui string changes as suggested by Bart & Teo

[1] https://git.reviewboard.kde.org/r/103752/

FEATURE: 280526
FEATURE: 264681
CCBUG: 291722
BUG: 263775
FIXED-IN: 2.6
REVIEW: 104213
DIGEST: Feature: much improved transcoding

Transcoding::Property: remove NUMERIC, LIST, TEXT types

These types were not used since Teo reworked all encoders to use the
TRADEOFF type. Remove them and associated code to make codebase cleaner
so that new code doesn't need to introduce case statements in switches
that will be never used, thus error-prone.

Individual types can be resurrected from this commit if there is a need
for them in future.

CollectionLocation: display source dialog in next mainloop iteration

This is to make CollectionLocation::prepareCopy/Move() return fast as
it advertises and not after several seconds when a modal dialog is
shown.


Diffs (updated)
-

  ChangeLog bd777eadf39aec71efb74dfed7502f564553d998 
  src/CMakeLists.txt 4241e69000c8b7fb944e4c86ddff3128829fb381 
  src/browsers/CollectionTreeView.h 26ac68a55242d52cdb1d789cef839643b56ccf5a 
  src/browsers/CollectionTreeView.cpp 7b3dd81b1cfd0fbb4d74b7eb71552a4ad92d74e5 
  src/browsers/filebrowser/FileView.h 890b9f458e7ae504a52019b8f41e3e6d2ba218a5 
  src/browsers/filebrowser/FileView.cpp 
425641af15e66c2670bce719eff1615f416ad6b7 
  src/configdialog/dialogs/CollectionConfig.cpp 
7704fb9fab5a09c4635c1ec7526ae05047df0c6f 
  src/configdialog/dialogs/CollectionConfig.ui 
d0ccdb33

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Teo Mrnjavac]

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

Ship it!


I believe it is completely justified to introduce a capability for this. Nice 
work!
Also, thanks for documenting your code :)
I've given it a spin and it seems to work, it's a ship it from me.

- Teo Mrnjavac


On March 9, 2012, 11:31 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
> ---
> 
> (Updated March 9, 2012, 11:31 p.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> ---
> 
> Rework transcoding: remember encoder, transcode on move, cleaner code
> 
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
> 
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
> 
> Why the hell the new Capability?
> 
> 
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferred transcoding
> paremeters etc.
> 
> I simply didn't want to pollute already over-crowded CollectionLocation
> with three more methods used by only a few subclasses. On the other
> hand, TranscodeCapability is not the central idea of this patch and I
> can factor it into CollectionLocation should there be a voice supporting
> it.
> 
> [1] https://git.reviewboard.kde.org/r/103752/
> 
> FEATURE: 280526
> FEATURE: 264681
> CCBUG: 291722
> BUG: 263775
> FIXED-IN: 2.6
> REVIEW: TODO
> DIGEST: Feature: much improved transcoding
> 
> --
> Next commit squelched for the purpose of review board
> --
> 
> Transcoding::Property: remove NUMERIC, LIST, TEXT types
> 
> These types were not used since Teo reworked all encoders to use the
> TRADEOFF type. Remove them and associated code to make codebase cleaner
> so that new code doesn't need to introduce case statements in switches
> that will be never used, thus error-prone.
> 
> Individual types can be resurrected from this commit if there is a need
> for them in future.
> 
> 

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Teo Mrnjavac]

> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > Screenshot: Revamped Transcode dialog
> > 
> >
> > This is hard to understand and contains some language errors. Perhaps 
> > "Use this for next tracks" ?
> 
> Matěj Laitl wrote:
> I particularly suck at english. :) I wanted to justify that this option 
> will be used for both copying and moving and that the setting is used just 
> for the particular collection. Additionally, even the "Just copy" will be 
> remembered. All suggestions for the GUI string that will make these clear are 
> welcome.
> 
> Bart Cerneels wrote:
> It's still not clear to me. But perhaps just "Save settings" ?

How about "Remember this choice for next time" ?


> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > Screenshot: Changes to the Configure Collection dialog
> > 
> >
> > If there are <= 3 options, don't use a combobox.
> 
> Matěj Laitl wrote:
> Yup, there are 2 or 3. Should I use radio buttons?
> 
> Bart Cerneels wrote:
> Use a groupbox: http://qt-project.org/doc/qt-4.8/widgets-groupbox.html
> 
> It might make the dialog to tall. Might not be worth it for such an 
> advanced feature.  Add a screenshot please so we can see what works. Please :)

I think a combobox works fine here. Also I'd make the label "Transcode tracks" 
without a colon at the end, with the choices: "Always", "Ask before each 
transfer", "Never".


- Teo


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


On March 9, 2012, 11:31 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
> ---
> 
> (Updated March 9, 2012, 11:31 p.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> ---
> 
> Rework transcoding: remember encoder, transcode on move, cleaner code
> 
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
> 
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
> 
> Why the hell the new Capability?
> 
> 
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferre

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Bart Cerneels]

> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > Screenshot: Changes to the Configure Collection dialog
> > 
> >
> > If there are <= 3 options, don't use a combobox.
> 
> Matěj Laitl wrote:
> Yup, there are 2 or 3. Should I use radio buttons?

Use a groupbox: http://qt-project.org/doc/qt-4.8/widgets-groupbox.html

It might make the dialog to tall. Might not be worth it for such an advanced 
feature.  Add a screenshot please so we can see what works. Please :)


> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > Screenshot: Revamped Transcode dialog
> > 
> >
> > This is hard to understand and contains some language errors. Perhaps 
> > "Use this for next tracks" ?
> 
> Matěj Laitl wrote:
> I particularly suck at english. :) I wanted to justify that this option 
> will be used for both copying and moving and that the setting is used just 
> for the particular collection. Additionally, even the "Just copy" will be 
> remembered. All suggestions for the GUI string that will make these clear are 
> welcome.

It's still not clear to me. But perhaps just "Save settings" ?


- Bart


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


On March 9, 2012, 11:31 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
> ---
> 
> (Updated March 9, 2012, 11:31 p.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> ---
> 
> Rework transcoding: remember encoder, transcode on move, cleaner code
> 
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
> 
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
> 
> Why the hell the new Capability?
> 
> 
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferred transcoding
> paremeters etc.
> 
> I simply didn't want to pollute already over-crowded CollectionLocation
> with three more methods used by only a few subclasses. On the other
> hand, TranscodeCapability is not the central idea of this patch and I
> can factor it into CollectionLocation should th

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Matěj Laitl]

> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > I think in this case the use of a Capability is completely justified. It's 
> > the Capabilities that just add complexity that are problematic.

Thanks for review!


> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > Screenshot: Changes to the Configure Collection dialog
> > 
> >
> > If there are <= 3 options, don't use a combobox.

Yup, there are 2 or 3. Should I use radio buttons?


> On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
> > Screenshot: Revamped Transcode dialog
> > 
> >
> > This is hard to understand and contains some language errors. Perhaps 
> > "Use this for next tracks" ?

I particularly suck at english. :) I wanted to justify that this option will be 
used for both copying and moving and that the setting is used just for the 
particular collection. Additionally, even the "Just copy" will be remembered. 
All suggestions for the GUI string that will make these clear are welcome.


- Matěj


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


On March 9, 2012, 11:31 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
> ---
> 
> (Updated March 9, 2012, 11:31 p.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> ---
> 
> Rework transcoding: remember encoder, transcode on move, cleaner code
> 
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
> 
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
> 
> Why the hell the new Capability?
> 
> 
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferred transcoding
> paremeters etc.
> 
> I simply didn't want to pollute already over-crowded CollectionLocation
> with three more methods used by only a few subclasses. On the other
> hand, TranscodeCapability is not the central idea of this patch and I
> can factor it into CollectionLocation should there be a voice supporting
> it.
> 
> [1] https://git.reviewboard.kde.org/r/103752/
> 
> FEATURE: 280526
> FEATURE: 264681
> CCB

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Bart Cerneels]

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

Ship it!


I think in this case the use of a Capability is completely justified. It's the 
Capabilities that just add complexity that are problematic.


Screenshot: Changes to the Configure Collection dialog

If there are <= 3 options, don't use a combobox.


Screenshot: Revamped Transcode dialog

This is hard to understand and contains some language errors. Perhaps "Use 
this for next tracks" ?

- Bart Cerneels


On March 9, 2012, 11:31 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
> ---
> 
> (Updated March 9, 2012, 11:31 p.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> ---
> 
> Rework transcoding: remember encoder, transcode on move, cleaner code
> 
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
> 
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
> 
> Why the hell the new Capability?
> 
> 
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferred transcoding
> paremeters etc.
> 
> I simply didn't want to pollute already over-crowded CollectionLocation
> with three more methods used by only a few subclasses. On the other
> hand, TranscodeCapability is not the central idea of this patch and I
> can factor it into CollectionLocation should there be a voice supporting
> it.
> 
> [1] https://git.reviewboard.kde.org/r/103752/
> 
> FEATURE: 280526
> FEATURE: 264681
> CCBUG: 291722
> BUG: 263775
> FIXED-IN: 2.6
> REVIEW: TODO
> DIGEST: Feature: much improved transcoding
> 
> --
> Next commit squelched for the purpose of review board
> --
> 
> Transcoding::Property: remove NUMERIC, LIST, TEXT types
> 
> These types were not used since Teo reworked all encoders to use the
> TRADEOFF type. Remove them and 

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Matěj Laitl]

On 10. 3. 2012 Julian Simioni wrote:
> Hi Matěj,
> Side question: you refer to an "in-the-works" ipod collection. Is this work
> in the main amarok git repository? If not, can you tell me where to find
> it?

You can find it in the ipod-rewrite branch (which contains reworked 
transcoding) of my Amarok repository clone [1]. Then you have to enabled it 
(and disable the old one) in Amarok Configuration -> plugins.

It is almost ready now, I'd be happy if you tested it and reported back to me.

[1] 
http://quickgit.kde.org/index.php?p=clones%2Famarok%2Flaitl%2Famarok.git&a=shortlog&h=refs/heads/ipod-
rewrite

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

Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Julian Simioni]

Hi Matěj*,
*Side question: you refer to an "in-the-works" ipod collection. Is this
work in the main amarok git repository? If not, can you tell me where to
find it?

Thanks,
Julian

On Fri, Mar 9, 2012 at 3:31 PM, Matěj Laitl  wrote:

>This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104213/
>   Review request for Amarok and Teo Mrnjavac.
> By Matěj Laitl.
> Description
>
> Rework transcoding: remember encoder, transcode on move, cleaner code
>
> This is a major rework of transcoding feature that brings following
> user-visible changes to Amarok:
>  * Amarok can remember preferred transcoding configuration per each
>collection that supports transcoding. Therefore, the "Use default
>configuration" work-around can go away and the "Transcode or copy?"
>dialog can (and is) be one-step now. This preference can be changed
>in configuration.
>  * Transcoding is now supported even during the move operation. No
>worries, only successfully transcoded tracks are removed from their
>original location.
>  * Only formats playable on the target collection are offered. Already
>used & tested in yet-to-be-merged iPod collection rewrite.
>  * The "Organize Tracks" dialog title and progress bar operation name
>now more verbosely describe actual operation to prevent user
>mistakes.
>  * Double-transcode when ripping audio CDs that caused failures is
>avoided. (ChangeLog entry for this was miscredited to my earilier
>commit)
>
> Technically, following changes are made:
>  * many methods that accepted optional TranscodingConfiguration now
>either have it mandatory or not at all.
>  * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
>INVALID along with convenience methods isValid() and isJustCopy().
>This simplifies logic in many methods.
>  * CollectionLocation::prepare{Copy,Move}() now don't have optional
>TranscodingConfiguration parameter. Depending on target collection,
>CollectionLocation determines it automatically or asks user in
>showSourceDialog() (overridable). AudioCdCollectionLocation already
>overrides it.
>  * Collections that support transcoding now should expose
>TranscodeCapability which is used to a) indicate that transcoding
>is supported; b) query which file formats are playable on target
>collection; c) read & save & unset preferred transcoding parameters.
>
> Why the hell the new Capability?
> 
>
> Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
> introduced the new one? In ideal world Amarok would be able to transcode
> everything regardless of the target collection. This is however not
> doable witch current copyUrlToCollection() design - target collection
> needs to do non-trivial things such as re-reading file tags, accounting
> for different file name and space requirements etc. See my comments in
> [1]. We therefore need a way for target collection to indicate it
> supports transcoding (in order not to fool user). Some collection
> locations such as TrashCollectionLocation should even intentionally
> disallow transcoding. Additionally, we want to be able to query
> supported destination file formats, to save preferred transcoding
> paremeters etc.
>
> I simply didn't want to pollute already over-crowded CollectionLocation
> with three more methods used by only a few subclasses. On the other
> hand, TranscodeCapability is not the central idea of this patch and I
> can factor it into CollectionLocation should there be a voice supporting
> it.
>
> [1] https://git.reviewboard.kde.org/r/103752/
>
> FEATURE: 280526
> FEATURE: 264681
> CCBUG: 291722
> BUG: 263775
> FIXED-IN: 2.6
> REVIEW: TODO
> DIGEST: Feature: much improved transcoding
>
> --
> Next commit squelched for the purpose of review board
> --
>
> Transcoding::Property: remove NUMERIC, LIST, TEXT types
>
> These types were not used since Teo reworked all encoders to use the
> TRADEOFF type. Remove them and associated code to make codebase cleaner
> so that new code doesn't need to introduce case statements in switches
> that will be never used, thus error-prone.
>
> Individual types can be resurrected from this commit if there is a need
> for them in future.
>
> --
> Next commit squelched for the purpose of review board
> --
>
> CollectionLocation: display source dialog in next mainloop iteration
>
> This is to make CollectionLocation::prepareCopy/Move() return fast as
> it advertises and not after several seconds when a modal dialog is
> shown.
>
>   Testing
>
> W

Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

[Matěj Laitl]

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

Review request for Amarok and Teo Mrnjavac.


Description
---

Rework transcoding: remember encoder, transcode on move, cleaner code

This is a major rework of transcoding feature that brings following
user-visible changes to Amarok:
 * Amarok can remember preferred transcoding configuration per each
   collection that supports transcoding. Therefore, the "Use default
   configuration" work-around can go away and the "Transcode or copy?"
   dialog can (and is) be one-step now. This preference can be changed
   in configuration.
 * Transcoding is now supported even during the move operation. No
   worries, only successfully transcoded tracks are removed from their
   original location.
 * Only formats playable on the target collection are offered. Already
   used & tested in yet-to-be-merged iPod collection rewrite.
 * The "Organize Tracks" dialog title and progress bar operation name
   now more verbosely describe actual operation to prevent user
   mistakes.
 * Double-transcode when ripping audio CDs that caused failures is
   avoided. (ChangeLog entry for this was miscredited to my earilier
   commit)

Technically, following changes are made:
 * many methods that accepted optional TranscodingConfiguration now
   either have it mandatory or not at all.
 * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
   INVALID along with convenience methods isValid() and isJustCopy().
   This simplifies logic in many methods.
 * CollectionLocation::prepare{Copy,Move}() now don't have optional
   TranscodingConfiguration parameter. Depending on target collection,
   CollectionLocation determines it automatically or asks user in
   showSourceDialog() (overridable). AudioCdCollectionLocation already
   overrides it.
 * Collections that support transcoding now should expose
   TranscodeCapability which is used to a) indicate that transcoding
   is supported; b) query which file formats are playable on target
   collection; c) read & save & unset preferred transcoding parameters.

Why the hell the new Capability?


Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
introduced the new one? In ideal world Amarok would be able to transcode
everything regardless of the target collection. This is however not
doable witch current copyUrlToCollection() design - target collection
needs to do non-trivial things such as re-reading file tags, accounting
for different file name and space requirements etc. See my comments in
[1]. We therefore need a way for target collection to indicate it
supports transcoding (in order not to fool user). Some collection
locations such as TrashCollectionLocation should even intentionally
disallow transcoding. Additionally, we want to be able to query
supported destination file formats, to save preferred transcoding
paremeters etc.

I simply didn't want to pollute already over-crowded CollectionLocation
with three more methods used by only a few subclasses. On the other
hand, TranscodeCapability is not the central idea of this patch and I
can factor it into CollectionLocation should there be a voice supporting
it.

[1] https://git.reviewboard.kde.org/r/103752/

FEATURE: 280526
FEATURE: 264681
CCBUG: 291722
BUG: 263775
FIXED-IN: 2.6
REVIEW: TODO
DIGEST: Feature: much improved transcoding

--
Next commit squelched for the purpose of review board
--

Transcoding::Property: remove NUMERIC, LIST, TEXT types

These types were not used since Teo reworked all encoders to use the
TRADEOFF type. Remove them and associated code to make codebase cleaner
so that new code doesn't need to introduce case statements in switches
that will be never used, thus error-prone.

Individual types can be resurrected from this commit if there is a need
for them in future.

--
Next commit squelched for the purpose of review board
--

CollectionLocation: display source dialog in next mainloop iteration

This is to make CollectionLocation::prepareCopy/Move() return fast as
it advertises and not after several seconds when a modal dialog is
shown.


Diffs
-

  ChangeLog 6f318678272275bb6de35fd8cd6355dd9c175f2d 
  src/CMakeLists.txt 4241e69000c8b7fb944e4c86ddff3128829fb381 
  src/browsers/CollectionTreeView.h 26ac68a55242d52cdb1d789cef839643b56ccf5a 
  src/browsers/CollectionTreeView.cpp 7b3dd81b1cfd0fbb4d74b7eb71552a4ad92d74e5 
  src/browsers/filebrowser/FileVie