Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
---
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
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
---
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 4241e69000c8b7fb944e4c86ddff3128829fb381
src/browsers/CollectionTreeView.h
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
---
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.
--
Next commit squelched for the purpose of review board
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
---
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
http://git.reviewboard.kde.org//r/104213/#scomment32
If there are = 3 options, don't use a combobox.
Screenshot: Revamped Transcode dialog
http://git.reviewboard.kde.org//r/104213/#scomment33
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 associated code to make codebase cleaner
so that new code doesn't need to introduce case statements in
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
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
http://git.reviewboard.kde.org
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
http://git.reviewboard.kde.org
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
CCBUG: 291722
BUG: 263775
FIXED-IN: 2.6
REVIEW: TODO
DIGEST: Feature: much improved transcoding
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
Screenshot: Changes to the Configure Collection dialog
http://git.reviewboard.kde.org
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
http://git.reviewboard.kde.org
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 there be a voice supporting
it.
[1] https://git.reviewboard.kde.org/r/103752/
FEATURE: 280526
FEATURE: 264681
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
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.gita=shortlogh=refs/heads/ipod- rewrite Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
---
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/FileView.h
