Re: Review Request 110036: WIP - Simple equalizer scripting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/110036/ --- (Updated Jan. 5, 2014, 5:06 p.m.) Status -- This change has been discarded. Review request for Amarok. Repository: amarok Description --- A new, unambitious attempt at adding equalizer functions to the scripting interface. Adds to EngineController the functions: * QString eqPreset() * void eqApplyPreset(QString name) And the signal: eqPresetApplied(QString name) Adds to AmarokEngineScript the same functions set up as a property. Diffs - src/EngineController.cpp 28fb256 src/scriptengine/AmarokEngineScript.h f1cdb8c src/scriptengine/AmarokEngineScript.cpp 4d52bbe src/EngineController.h 5de4beb Diff: https://git.reviewboard.kde.org/r/110036/diff/ Testing --- Quick check using the script console that presets can be changed. The preset does get applied but it won't show in the EqualizerDialog. Possible other bugs. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 110036: WIP - Simple equalizer scripting
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110036/ --- (Updated April 18, 2013, 8:03 a.m.) Review request for Amarok. Description --- A new, unambitious attempt at adding equalizer functions to the scripting interface. Adds to EngineController the functions: * QString eqPreset() * void eqApplyPreset(QString name) And the signal: eqPresetApplied(QString name) Adds to AmarokEngineScript the same functions set up as a property. Diffs (updated) - src/EngineController.cpp 28fb256 src/scriptengine/AmarokEngineScript.h f1cdb8c src/scriptengine/AmarokEngineScript.cpp 4d52bbe src/EngineController.h 5de4beb Diff: http://git.reviewboard.kde.org/r/110036/diff/ Testing --- Quick check using the script console that presets can be changed. The preset does get applied but it won't show in the EqualizerDialog. Possible other bugs. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 110036: WIP - Simple equalizer scripting
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110036/ --- (Updated April 18, 2013, 7:50 a.m.) Review request for Amarok. Description --- A new, unambitious attempt at adding equalizer functions to the scripting interface. Adds to EngineController the functions: * QString eqPreset() * void eqApplyPreset(QString name) And the signal: eqPresetApplied(QString name) Adds to AmarokEngineScript the same functions set up as a property. Diffs (updated) - src/scriptengine/AmarokEngineScript.cpp 4d52bbe src/scriptengine/AmarokEngineScript.h f1cdb8c src/EngineController.cpp 28fb256 src/EngineController.h 5de4beb Diff: http://git.reviewboard.kde.org/r/110036/diff/ Testing --- Quick check using the script console that presets can be changed. The preset does get applied but it won't show in the EqualizerDialog. Possible other bugs. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request 110036: WIP - Simple equalizer scripting
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110036/ --- Review request for Amarok. Summary (updated) - WIP - Simple equalizer scripting Description (updated) --- A new, unambitious attempt at adding equalizer functions to the scripting interface. Adds to EngineController the functions: * QString eqPreset() * void eqApplyPreset(QString name) And the signal: eqPresetApplied(QString name) Adds to AmarokEngineScript the same functions set up as a property. Diffs (updated) - src/EngineController.h 5de4beb src/EngineController.cpp 28fb256 src/scriptengine/AmarokEngineScript.h f1cdb8c src/scriptengine/AmarokEngineScript.cpp 4d52bbe Diff: http://git.reviewboard.kde.org/r/110036/diff/ Testing (updated) --- Quick check using the script console that presets can be changed. The preset does get applied but it won't show in the EqualizerDialog. Possible other bugs. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: WIP - Dedicated equalizer controller
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106511/ --- (Updated Sept. 21, 2012, 12:45 a.m.) Review request for Amarok. Changes --- Fixed issue of version two only including changes from one file. Description --- This patch moves much of the Equalizer Dialogues internal behaviour into a dedicated object accessed via The::equalizer()*. Any component of Amarok looking to adjust equalizer levels (i.e. The equalizer scripting support I'll resubmit some day) should use this interface. I'm considering as my next steps, merging the EqualizerPresets class with the EqualizerController class and possibly removing dependence on AmarokConfig to make the actual changes.. *The equalizer dialogue is accessed via The::equalizerDialog() Diffs (updated) - src/CMakeLists.txt 8596144 src/EqualizerController.h PRE-CREATION src/EqualizerController.cpp PRE-CREATION src/MainWindow.cpp 0530fd7 src/core/support/Components.h c38977c src/core/support/Components.cpp 22e5de0 src/dialogs/EqualizerDialog.h 9dad055 src/dialogs/EqualizerDialog.cpp 3a63fe6 Diff: http://git.reviewboard.kde.org/r/106511/diff/ Testing --- Checked that Amarok compiles and that the equalizer dialogue still works. Found that enabling/dis-enabling the equalizer forces the track to freeze or restart. Pressing stop and then play will make it continue with the correct preset enabled. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: WIP - Dedicated equalizer controller
> On Sept. 20, 2012, 3:21 p.m., Matěj Laitl wrote:
> > src/EqualizerController.h, lines 37-44
> > <http://git.reviewboard.kde.org/r/106511/diff/1/?file=86406#file86406line37>
> >
> > All methods lack documentation, which is unacceptable for a component.
> >
> > From the names it seems that EqualizerController would solve 2 purposes:
> >
> > a) getting and setting the currently used equalizer preset
> > b) store for equalizer presets
> >
> > Is it a good idea to combine these into one controller? Maybe yes, but
> > I'd like to hear the reasoning.
> >
> > I also dislike the whole EqulizerPresets class. Why there's no
> > EqualizerPreset class that would contain the values and name? I think that
> > it would make EqualizerController interface cleaner.
> >
> > Comments on individual methods:
> > * int presetNum() const; void setPreset(int number) const: seems
> > redundant with the name-base methods.
> > * QString presetName() const; should be named currentPresetName()?
> > Ideally this would be EqualizerPreset currentPreset() const;
> > * void setPreset(QString name) const; should be named activatePreset?
> > I would love it it would accept "const EqualizerPreset &preset" instead.
> >
> > * const EqualizerPresets presets() const; -> QList
> > savedPresets() const;? Also returning const by value has no sense.
> > * void setPresetValues(const QString name, const QList& values);
> > -> void savePreset( const EqualizerPreset &preset );?
> > * bool deletePreset(const QString name); Why bool? Should it take name
> > or const EqualizerPreset &preset as an argument?
> > * bool restorePreset(const QString name); what does this do?
> >
> > My suggestion for the EqualizerPreset class:
> >
> > .h file:
> >
> > class EqualizerPreset
> > {
> > public:
> > EqualizerPreset( const QString &name, const QList &values )
> >
> > QString name() const;
> > void setName();
> >
> > QList values() const;
> > void setValues( const QList &values );
> > void setValue( int index, int value ); // convenience
> >
> > operator==( const EqualizerPreset &other ) const;
> >
> > private:
> > class Private;
> > QSharedDataPointer d;
> > }
> >
> > .cpp file:
> >
> > class EqualizerPreset::Private {
> > public:
> > QString name;
> > QList values;
> > }
I'm going to have to address this comment as if it where several. Here goes.
1 (No documentation):
Just updated the patch to fix that. Hope it is nice and clear but I suspect
not.
2 (Purpose):
I'm still vaguely intending on writing a script that changes the equaliser
preset when the track changes, which means adding in support
for scripting the equaliser. At the moment that is impossible since the
presets are kept in a private member of the equaliser dialogue.
Much of the getting and setting is done in there as well. The obvious
solution was to make both the dialogue and scripting interface
front ends on a single controller.
3 (EqualizerPresets vs. EqualizerPreset)
Like I mentioned before one of my next steps is to completely remove
EqualizerPresets after
reimplementing its behaviour in EqualizerController.
Your suggestion for an EqualizerPreset class could really clean up this
interface and give a good incentive to further clean up
the equaliser dialogue. I would probably write it a little differently
however.
4 (*Num classes)
They seemed necessary at the time due to AmarokConfig doing everything
according to index numbers. But I could probaby remove
these without causing any inconvenience.
5 (Renamings)
Consider it done.
6 (const makes no sense)
I was putting const in pretty much everywhere else. Also thinking in Scala.
7 (Why bool?)
Because the equaliser dialogue wouldn't compile without getting a bool.
8 (What does restore do?)
I'm still not certain. The dialogue needed it but if it does what I think
it does from the updated patch* then it probably
isn't needed as the current behaviour of the dialogue seems to obsolete it.
*Which I just realised only covers one .h file
- Ryan
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde
Re: Review Request: WIP - Dedicated equalizer controller
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106511/ --- (Updated Sept. 20, 2012, 11:24 p.m.) Review request for Amarok. Changes --- Added documentation to the patch. Note that the explanation of restorePreset() is a guess. The equaliser dialogue insist on having it. However if it servers the purpose that I think it does, we can safely remove it. EqualizerPresets::eqCfgRestorePreset is a horrid function anyway. Description --- This patch moves much of the Equalizer Dialogues internal behaviour into a dedicated object accessed via The::equalizer()*. Any component of Amarok looking to adjust equalizer levels (i.e. The equalizer scripting support I'll resubmit some day) should use this interface. I'm considering as my next steps, merging the EqualizerPresets class with the EqualizerController class and possibly removing dependence on AmarokConfig to make the actual changes.. *The equalizer dialogue is accessed via The::equalizerDialog() Diffs (updated) - src/EqualizerController.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/106511/diff/ Testing --- Checked that Amarok compiles and that the equalizer dialogue still works. Found that enabling/dis-enabling the equalizer forces the track to freeze or restart. Pressing stop and then play will make it continue with the correct preset enabled. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: WIP - Dedicated equalizer controller
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106511/ --- Review request for Amarok. Description --- This patch moves much of the Equalizer Dialogues internal behaviour into a dedicated object accessed via The::equalizer()*. Any component of Amarok looking to adjust equalizer levels (i.e. The equalizer scripting support I'll resubmit some day) should use this interface. I'm considering as my next steps, merging the EqualizerPresets class with the EqualizerController class and possibly removing dependence on AmarokConfig to make the actual changes.. *The equalizer dialogue is accessed via The::equalizerDialog() Diffs - src/CMakeLists.txt 8596144 src/EqualizerController.h PRE-CREATION src/EqualizerController.cpp PRE-CREATION src/MainWindow.cpp 0530fd7 src/core/support/Components.h c38977c src/core/support/Components.cpp 22e5de0 src/dialogs/EqualizerDialog.h 9dad055 src/dialogs/EqualizerDialog.cpp 3a63fe6 Diff: http://git.reviewboard.kde.org/r/106511/diff/ Testing --- Checked that Amarok compiles and that the equalizer dialogue still works. Found that enabling/dis-enabling the equalizer forces the track to freeze or restart. Pressing stop and then play will make it continue with the correct preset enabled. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Support for equalizer scripts and plugins. WIP.
On Aug. 16, 2012, 10:18 a.m., Ryan McCoskrie wrote: > > How do we continue? Is this ready to be submitted? What is the actual > > functionality? Can you also add a line for the ChangeLog.txt? Will try to get a related patch (equalizer controller) into Amarok first. With that done Adding a scripting interface will only require a small, one-issue patch. - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102798/#review17511 --- On Nov. 18, 2011, 11:47 p.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102798/ > --- > > (Updated Nov. 18, 2011, 11:47 p.m.) > > > Review request for Amarok. > > > Description > --- > > This patch is the beginning of an effort to make a generic equalizer > interface for plugins and scripts to access. > > 0: Gives the EngineController controller class new functions for examining, > choosing and altering presets. > 0: AmarokEngineScript provides functions to access the simpler equalizer > functions in EngineController. > 0: EqualizerDialog now uses the The::engineController() as a back end. > > WARNING: I don't properly understand how the equalizer works. The largest > changes in this patch are mostly > code moved from EqualizerDialog to EngineController. It's quite ugly in > places. > > > Diffs > - > > src/ActionClasses.cpp 5d87f4b > src/EngineController.h 3da2f23 > src/EngineController.cpp 6060137 > src/dialogs/EqualizerDialog.h 9dad055 > src/dialogs/EqualizerDialog.cpp ddcd300 > src/scriptengine/AmarokEngineScript.h f1cdb8c > src/scriptengine/AmarokEngineScript.cpp 395c504 > > Diff: http://git.reviewboard.kde.org/r/102798/diff/ > > > Testing > --- > > 0: Briefly tested the Amarok.Engine.eqChangeCurrentPreset() function in the > script console. Works as expected. > 0: Amarok.Engine.eqBandsFreq() seems to return no value. > > > # First Revision # > > 0: Opened script console and Equalizer Dialog. Used both the dialog and > Amarok.Engine.currentPreset property > to alter the equalizer. Everything appears to work properly. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Extend the scope of the playground
> On Aug. 22, 2012, 8:25 a.m., Bart Cerneels wrote: > > I would like to see the code you are working on that has a need for this > > change. I still think that it should go directly in core. > > Matěj Laitl wrote: > Yup. I don't really think we should merge this. `git branch` is much > better tool than CMake variables and C preprocessor. It's not so much that the code needs this change as I got told off last time* for not putting it under the playground. *Long story short, I lost my work - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103999/#review17840 --- On Aug. 19, 2012, 10:09 p.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103999/ > --- > > (Updated Aug. 19, 2012, 10:09 p.m.) > > > Review request for Amarok. > > > Description > --- > > This is infrastructure for future patches of mine (specifically equalizer > scripting now that I am back on it). > The intention of this patch is to allow for the /playground directory to > compile code directly into the > Amarok binary. The benefit this has to the Amarok project is that stable > releases can include some experimental > featues that power users can opt into using. > > Changes made to this patch: > 0 Separated from a minor clean up of the playground > 0 Separated from my equalizer scripting code (How on Earth did I overlook > that?) > 0 Added a brief tutorial on compiling in playground code > 0 Now address all platforms, not just those using X11 > 0 Now intended for serious consideration of shipping. > > > Diffs > - > > CMakeLists.txt ebb8064 > playground/PLAYER_BINARY.txt PRE-CREATION > playground/src/CMakeLists.txt ed740ec > src/CMakeLists.txt 8596144 > > Diff: http://git.reviewboard.kde.org/r/103999/diff/ > > > Testing > --- > > Checked that the code compiles with the playground option enabled. > Since there is no active code, this should be sufficiant. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Extend the scope of the playground
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103999/ --- (Updated Aug. 19, 2012, 10:09 p.m.) Review request for Amarok. Description (updated) --- This is infrastructure for future patches of mine (specifically equalizer scripting now that I am back on it). The intention of this patch is to allow for the /playground directory to compile code directly into the Amarok binary. The benefit this has to the Amarok project is that stable releases can include some experimental featues that power users can opt into using. Changes made to this patch: 0 Separated from a minor clean up of the playground 0 Separated from my equalizer scripting code (How on Earth did I overlook that?) 0 Added a brief tutorial on compiling in playground code 0 Now address all platforms, not just those using X11 0 Now intended for serious consideration of shipping. Diffs (updated) - CMakeLists.txt ebb8064 playground/PLAYER_BINARY.txt PRE-CREATION playground/src/CMakeLists.txt ed740ec src/CMakeLists.txt 8596144 Diff: http://git.reviewboard.kde.org/r/103999/diff/ Testing (updated) --- Checked that the code compiles with the playground option enabled. Since there is no active code, this should be sufficiant. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Remove obsolete dependancies from playground
> On Aug. 18, 2012, 10:17 a.m., Matěj Laitl wrote: > > Ship it! Just to be sure, your testing "the code compiles" was with > > ENABLE_PLAYGROUND enabled, right? Do you heve developer status on > > identity.k.o to commit yourself? Yes I did compile it with ENABLE_PLAYGROUND, this patch is actually just my old extended playground patch with the insertions removed. No I don't have developer status and until I get used to team work that a _very_ good thing, - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106067/#review17649 --- On Aug. 18, 2012, 1:26 a.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106067/ > --- > > (Updated Aug. 18, 2012, 1:26 a.m.) > > > Review request for Amarok. > > > Description > --- > > Removed several dependancies from /playground/CMakeLists.txt that > /CMakeLists.txt and /src/CMakeLists.txt handle better. > > > Diffs > - > > playground/CMakeLists.txt e96aa3f > > Diff: http://git.reviewboard.kde.org/r/106067/diff/ > > > Testing > --- > > Checked that it compiles. No discrernable differences. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Extend the scope of the playground
> On Aug. 16, 2012, 11:14 a.m., Matěj Laitl wrote: > > The autor says he doesn't intend to see this merged, to I presume we can > > close this now. > > > > Ryan, as Bart says, there's some unrelated cleanup in this patch, could you > > please submit that as a separate review? Tracks. Done. - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103999/#review17516 --- On Feb. 17, 2012, 2:53 a.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103999/ > --- > > (Updated Feb. 17, 2012, 2:53 a.m.) > > > Review request for Amarok. > > > Description > --- > > This is infrastructure for future patches of mine. It's intended to (after > some revising) make it relatively easy to develop code that is (mostly) in > the playground directory but is (by necessity) compiled into the player > binary. > > Note: I'm only looking for criticism and have no intention of seeing this > shipped. > > > Diffs > - > > CMakeLists.txt d47c28b > playground/CMakeLists.txt e96aa3f > playground/src/CMakeLists.txt ed740ec > src/App.cpp 2f1837c > src/CMakeLists.txt 4241e69 > > Diff: http://git.reviewboard.kde.org/r/103999/diff/ > > > Testing > --- > > Checked that the code compiles. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Remove obsolete dependancies from playground
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106067/ --- Review request for Amarok. Description --- Removed several dependancies from /playground/CMakeLists.txt that /CMakeLists.txt and /src/CMakeLists.txt Diffs - playground/CMakeLists.txt e96aa3f Diff: http://git.reviewboard.kde.org/r/106067/diff/ Testing --- Checked that it compiles. No discrernable differences. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Remove obsolete dependancies from playground
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106067/ --- (Updated Aug. 18, 2012, 1:26 a.m.) Review request for Amarok. Description (updated) --- Removed several dependancies from /playground/CMakeLists.txt that /CMakeLists.txt and /src/CMakeLists.txt handle better. Diffs - playground/CMakeLists.txt e96aa3f Diff: http://git.reviewboard.kde.org/r/106067/diff/ Testing --- Checked that it compiles. No discrernable differences. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Support for equalizer scripts and plugins. WIP.
> On Aug. 16, 2012, 10:18 a.m., Ralf Engels wrote: > > Is this still WIP? Well it's been a work in progress in the same sense that I'm learning to play the harmonica that has been sitting idly on my desk for a month or two. Now that I'm getting emails about it however I'll start afresh. - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102798/#review17511 --- On Nov. 18, 2011, 11:47 p.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102798/ > --- > > (Updated Nov. 18, 2011, 11:47 p.m.) > > > Review request for Amarok. > > > Description > --- > > This patch is the beginning of an effort to make a generic equalizer > interface for plugins and scripts to access. > > 0: Gives the EngineController controller class new functions for examining, > choosing and altering presets. > 0: AmarokEngineScript provides functions to access the simpler equalizer > functions in EngineController. > 0: EqualizerDialog now uses the The::engineController() as a back end. > > WARNING: I don't properly understand how the equalizer works. The largest > changes in this patch are mostly > code moved from EqualizerDialog to EngineController. It's quite ugly in > places. > > > Diffs > - > > src/ActionClasses.cpp 5d87f4b > src/EngineController.h 3da2f23 > src/EngineController.cpp 6060137 > src/dialogs/EqualizerDialog.h 9dad055 > src/dialogs/EqualizerDialog.cpp ddcd300 > src/scriptengine/AmarokEngineScript.h f1cdb8c > src/scriptengine/AmarokEngineScript.cpp 395c504 > > Diff: http://git.reviewboard.kde.org/r/102798/diff/ > > > Testing > --- > > 0: Briefly tested the Amarok.Engine.eqChangeCurrentPreset() function in the > script console. Works as expected. > 0: Amarok.Engine.eqBandsFreq() seems to return no value. > > > # First Revision # > #### > 0: Opened script console and Equalizer Dialog. Used both the dialog and > Amarok.Engine.currentPreset property > to alter the equalizer. Everything appears to work properly. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add composer button to wikipedia applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/#review11865 --- Just found out about the Wikipedia applet having separate language controls. This patch works fine. - Ryan McCoskrie On March 26, 2012, 6:50 a.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104327/ > --- > > (Updated March 26, 2012, 6:50 a.m.) > > > Review request for Amarok. > > > Description > --- > > Adds a new composer button for the Wikipedia applet. > > Currently only works in English. > > > This addresses bug 272982. > https://bugs.kde.org/show_bug.cgi?id=272982 > > > Diffs > - > > src/context/applets/wikipedia/WikipediaApplet.h 897a32c > src/context/applets/wikipedia/WikipediaApplet.cpp e326804 > src/context/applets/wikipedia/WikipediaApplet_p.h b0d6116 > src/context/engines/wikipedia/WikipediaEngine.cpp a4ee870 > > Diff: http://git.reviewboard.kde.org/r/104327/diff/ > > > Testing > --- > > Compiled Amarok, changed a file to have a different musician name to the > composer name and pressed button. Everything works as expected. > This patch is simply a case of copying, pasting, substituteing artist with > composer so new bugs are extremely unlikely. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add composer button to wikipedia applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/ --- (Updated March 26, 2012, 6:50 a.m.) Review request for Amarok. Changes --- Changed wikipedia search patterns to use i18nc() rather than QLatinString(). Changing application language doesn't seem to affect the wikipedia applet before or after this change so I can't call it a step backwards. Description --- Adds a new composer button for the Wikipedia applet. Currently only works in English. This addresses bug 272982. https://bugs.kde.org/show_bug.cgi?id=272982 Diffs (updated) - src/context/applets/wikipedia/WikipediaApplet.h 897a32c src/context/applets/wikipedia/WikipediaApplet.cpp e326804 src/context/applets/wikipedia/WikipediaApplet_p.h b0d6116 src/context/engines/wikipedia/WikipediaEngine.cpp a4ee870 Diff: http://git.reviewboard.kde.org/r/104327/diff/ Testing --- Compiled Amarok, changed a file to have a different musician name to the composer name and pressed button. Everything works as expected. This patch is simply a case of copying, pasting, substituteing artist with composer so new bugs are extremely unlikely. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add composer button to wikipedia applet
> On March 18, 2012, 10:22 a.m., Matěj Laitl wrote: > > src/context/engines/wikipedia/WikipediaEngine.cpp, line 462 > > <http://git.reviewboard.kde.org/r/104327/diff/1/?file=53706#file53706line462> > > > > Hmm, why don't we use i18nc() for creating the pattern here? Something > > with a very descriptive context string should do. This applies to all cases > > in the swich, not just this patch. The strings here are for fetching data from Wikipedia rather than displaying to the user. It seems best to me that such data should be hard coded. That said, I am new to writing translatable software so I'll do some research on how these functions work and take any subsequent advice. - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/#review11530 --- On March 18, 2012, 6:27 a.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104327/ > --- > > (Updated March 18, 2012, 6:27 a.m.) > > > Review request for Amarok. > > > Description > --- > > Adds a new composer button for the Wikipedia applet. > > Currently only works in English. > > > This addresses bug 272982. > https://bugs.kde.org/show_bug.cgi?id=272982 > > > Diffs > - > > src/context/applets/wikipedia/WikipediaApplet_p.h b0d6116 > src/context/engines/wikipedia/WikipediaEngine.cpp a4ee870 > src/context/applets/wikipedia/WikipediaApplet.h 897a32c > src/context/applets/wikipedia/WikipediaApplet.cpp e326804 > > Diff: http://git.reviewboard.kde.org/r/104327/diff/ > > > Testing > --- > > Compiled Amarok, changed a file to have a different musician name to the > composer name and pressed button. Everything works as expected. > This patch is simply a case of copying, pasting, substituteing artist with > composer so new bugs are extremely unlikely. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add composer button to wikipedia applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/ --- (Updated March 19, 2012, 8:55 p.m.) Review request for Amarok. Changes --- Clarified no tag error messages. Description --- Adds a new composer button for the Wikipedia applet. Currently only works in English. This addresses bug 272982. https://bugs.kde.org/show_bug.cgi?id=272982 Diffs (updated) - src/context/applets/wikipedia/WikipediaApplet.h 897a32c src/context/applets/wikipedia/WikipediaApplet.cpp e326804 src/context/applets/wikipedia/WikipediaApplet_p.h b0d6116 src/context/engines/wikipedia/WikipediaEngine.cpp a4ee870 Diff: http://git.reviewboard.kde.org/r/104327/diff/ Testing --- Compiled Amarok, changed a file to have a different musician name to the composer name and pressed button. Everything works as expected. This patch is simply a case of copying, pasting, substituteing artist with composer so new bugs are extremely unlikely. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Add composer button to wikipedia applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/ --- Review request for Amarok. Description --- Adds a new composer button for the Wikipedia applet. Currently only works in English. This addresses bug 272982. https://bugs.kde.org/show_bug.cgi?id=272982 Diffs - src/context/applets/wikipedia/WikipediaApplet_p.h b0d6116 src/context/engines/wikipedia/WikipediaEngine.cpp a4ee870 src/context/applets/wikipedia/WikipediaApplet.h 897a32c src/context/applets/wikipedia/WikipediaApplet.cpp e326804 Diff: http://git.reviewboard.kde.org/r/104327/diff/ Testing --- Compiled Amarok, changed a file to have a different musician name to the composer name and pressed button. Everything works as expected. This patch is simply a case of copying, pasting, substituteing artist with composer so new bugs are extremely unlikely. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated March 7, 2012, 11:39 p.m.) Review request for Amarok. Changes --- 0 Fixed final issues from trying to race self. Description --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs (updated) - src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f Diff: http://git.reviewboard.kde.org/r/102236/diff/ Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated March 7, 2012, 11:12 p.m.) Review request for Amarok. Changes --- 0 Removed all use of string concatenation 0 Changed from using context-free i18n() to i18nc() with context notes. 0 Proper use of artistName, trackName, etcetera variables. Description --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs (updated) - src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f Diff: http://git.reviewboard.kde.org/r/102236/diff/ Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated March 7, 2012, 10:30 p.m.) Review request for Amarok. Changes --- 0 Uses collection name. 0 Uses single inline function to generate list of track names 0 Possibly easier to translate Description --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs (updated) - src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f Diff: http://git.reviewboard.kde.org/r/102236/diff/ Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Bump the taglib dependency to 1.7
On Fri, 27 Jan 2012 03:24:51 Daniel Faust wrote: > actually taglib 1.7 is not needed, I just forgot to wrap the #include > around the #ifdef ... > > So I could either fix that and we can go back to taglib 1.6 or I'll remove > all the #ifdef ... that are already used. > I would suggest going back to 1.6 as 1.7 isn't yet available as a stable package on Mageia. -- Ryan McCoskrie North Canterbury, New Zealand sourcelinksnotes.comyr.com ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Amarok out of string freeze, let's plan features
On Thu, 22 Dec 2011 02:15:18 Bart Cerneels wrote: > It took a bit longer then first planned, but 2.5 is now released. > Like I mentioned before [1], it would be better for planning and > quality if we try to stick to a known list of features or enhancements > for each release. > > So what features do you have planned or would strongly suggest to make > it into 2.6? > I'll follow up later with my own shortlist. > I've got lots of codeing time coming up so I'll probably be able to get my second attempt at an equalizer scripting interface written and debuged by then. -- Ryan McCoskrie, North Canterbury, New Zealand http://sourcelinksnotes.comyr.com signature.asc Description: This is a digitally signed message part. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Where to place generic equalizer facilities.
I've been doing some on off work to make some standard functions for scripts and plugins to controll the equalizer through. So far I have been placing all of these in the EngineController class (and giving AmarokScriptEngine some wrap around functions) but the number of them is growing and I've started to wonder if I should make a new class to hold this behaviour? -- Ryan McCoskrie North Canterbury, New Zealand sourcelinksnotes.comyr.com ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Support for equalizer scripts and plugins. WIP.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102798/ --- (Updated Nov. 18, 2011, 11:47 p.m.) Review request for Amarok. Changes --- Remembered to update the diff this time. Description --- This patch is the beginning of an effort to make a generic equalizer interface for plugins and scripts to access. 0: Gives the EngineController controller class new functions for examining, choosing and altering presets. 0: AmarokEngineScript provides functions to access the simpler equalizer functions in EngineController. 0: EqualizerDialog now uses the The::engineController() as a back end. WARNING: I don't properly understand how the equalizer works. The largest changes in this patch are mostly code moved from EqualizerDialog to EngineController. It's quite ugly in places. Diffs (updated) - src/ActionClasses.cpp 5d87f4b src/EngineController.h 3da2f23 src/EngineController.cpp 6060137 src/dialogs/EqualizerDialog.h 9dad055 src/dialogs/EqualizerDialog.cpp ddcd300 src/scriptengine/AmarokEngineScript.h f1cdb8c src/scriptengine/AmarokEngineScript.cpp 395c504 Diff: http://git.reviewboard.kde.org/r/102798/diff/diff Testing --- 0: Briefly tested the Amarok.Engine.eqChangeCurrentPreset() function in the script console. Works as expected. 0: Amarok.Engine.eqBandsFreq() seems to return no value. # First Revision # 0: Opened script console and Equalizer Dialog. Used both the dialog and Amarok.Engine.currentPreset property to alter the equalizer. Everything appears to work properly. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Support for equalizer scripts and plugins. WIP.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102798/ --- (Updated Nov. 18, 2011, 8:54 p.m.) Review request for Amarok. Summary (updated) - Support for equalizer scripts and plugins. WIP. Description (updated) --- This patch is the beginning of an effort to make a generic equalizer interface for plugins and scripts to access. 0: Gives the EngineController controller class new functions for examining, choosing and altering presets. 0: AmarokEngineScript provides functions to access the simpler equalizer functions in EngineController. 0: EqualizerDialog now uses the The::engineController() as a back end. WARNING: I don't properly understand how the equalizer works. The largest changes in this patch are mostly code moved from EqualizerDialog to EngineController. It's quite ugly in places. Diffs - src/EngineController.h 3da2f23 src/EngineController.cpp 5bea5c6 src/dialogs/EqualizerDialog.h 9dad055 src/dialogs/EqualizerDialog.cpp ddcd300 src/scriptengine/AmarokEngineScript.h f1cdb8c src/scriptengine/AmarokEngineScript.cpp 395c504 Diff: http://git.reviewboard.kde.org/r/102798/diff/diff Testing (updated) --- 0: Briefly tested the Amarok.Engine.eqChangeCurrentPreset() function in the script console. Works as expected. 0: Amarok.Engine.eqBandsFreq() seems to return no value. # First Revision # 0: Opened script console and Equalizer Dialog. Used both the dialog and Amarok.Engine.currentPreset property to alter the equalizer. Everything appears to work properly. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] /: CMake warning on missing TagLib
On Thu, 10 Nov 2011 15:53:59 Sam Lade wrote: > It does work without TagLib (it'll guess tags from filenames, and other > functions work fine). You might want to note that when I was (unknowingly) using Amarok without taglib the filename guesser was trying to use track numbers as artist names, failing to detect albums and showing all manner of other strange bugs. -- Ryan McCoskrie North Canterbury, New Zealand sourcelinksnotes.comyr.com ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Warn against build without taglib
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103090/ --- Review request for Amarok. Description --- Makes CMake print a stern warning against building Amarok without support of taglib if it is attempted. Diffs - CMakeLists.txt 6fb0491 Diff: http://git.reviewboard.kde.org/r/103090/diff/diff Testing --- Built Amarok with this patch applied. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Initial look into equalizer scritping. Very experimental.
> On Oct. 7, 2011, 10:42 a.m., Myriam Schweingruber wrote: > > Since it is very experimental this belongs to /playground, please do not > > submit it for /src Duly noted. I'll add that to the development wiki for other first-timers. - Ryan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102798/#review7162 --- On Oct. 7, 2011, 2:43 a.m., Ryan McCoskrie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102798/ > --- > > (Updated Oct. 7, 2011, 2:43 a.m.) > > > Review request for Amarok. > > > Description > --- > > This patch is the beginning of an effort to make a generic equalizer > interface for plugins and scripts to access. > > 0: Gives the EngineController controller class three new functions for > examining and choosing the current preset. > 0: Started to add equalizer support into the class AmarokEngineScript. > 0: Began work to make the EqualizerDialog class a front end on the equalizer > facilities in EngineController. > > WARNING: Adding custom presets with this patch in place is almost certainly > not possible. > > > Diffs > - > > src/EngineController.h 3da2f23 > src/EngineController.cpp 5bea5c6 > src/dialogs/EqualizerDialog.h 9dad055 > src/dialogs/EqualizerDialog.cpp ddcd300 > src/scriptengine/AmarokEngineScript.h f1cdb8c > src/scriptengine/AmarokEngineScript.cpp 395c504 > > Diff: http://git.reviewboard.kde.org/r/102798/diff/diff > > > Testing > --- > > 0: Briefly tested the Amarok.Engine.eqChangeCurrentPreset() function in the > script console. Works as expected. > 0: Amarok.Engine.eqBandsFreq() seems to return no value. > > > Thanks, > > Ryan McCoskrie > > ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Initial look into equalizer scritping. Very experimental.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102798/ --- Review request for Amarok. Description --- This patch is the beginning of an effort to make a generic equalizer interface for plugins and scripts to access. 0: Gives the EngineController controller class three new functions for examining and choosing the current preset. 0: Started to add equalizer support into the class AmarokEngineScript. 0: Began work to make the EqualizerDialog class a front end on the equalizer facilities in EngineController. WARNING: Adding custom presets with this patch in place is almost certainly not possible. Diffs - src/EngineController.h 3da2f23 src/EngineController.cpp 5bea5c6 src/dialogs/EqualizerDialog.h 9dad055 src/dialogs/EqualizerDialog.cpp ddcd300 src/scriptengine/AmarokEngineScript.h f1cdb8c src/scriptengine/AmarokEngineScript.cpp 395c504 Diff: http://git.reviewboard.kde.org/r/102798/diff/diff Testing --- 0: Briefly tested the Amarok.Engine.eqChangeCurrentPreset() function in the script console. Works as expected. 0: Amarok.Engine.eqBandsFreq() seems to return no value. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated Sept. 10, 2011, 10:26 p.m.) Review request for Amarok. Changes --- Adjusted to always show path name before meta data. This way users can be certain of what they are doing while delete redundant files. Summary --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs (updated) - src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp 349464c Diff: http://git.reviewboard.kde.org/r/102236/diff Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Scripting interfaces and list values
I'm working on adding support for equalizer scripting but I have hit the
interesting snag some of the functions that I have wrapped around aren't
returning a value.
One good example is this:
QStringList AmarokEqualizerScript::getPresetNames() const
{
return AmarokConfig::equalizerPresetsNames();
}
It returns an empty array. Could someone point out what I am doing wrong?
I'm pretty sure I could get it all right once I'm put on the right course.
--
Ryan McCoskrie
North Canterbury, New Zealand
sourcelinksnotes.comyr.com
signature.asc
Description: This is a digitally signed message part.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- Review request for Amarok. Summary --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs - src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp 349464c Diff: http://git.reviewboard.kde.org/r/102236/diff Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Fix for bug #263693
On 8 August 2011 10:26, Sam Lade wrote: > > As Bart said, please submit patches to http://git.reviewboard.kde.org/ > (you'll need an account with identity.kde.org, if I remember correctly), > rather than the mailing list. It makes everything much neater and easier > to keep track of - things on the list can get lost far too easily. > > I tried that yesterday, but I can't figure out how to get it past being listed as a draft. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Fix for bug #263693
This patch adjust the 'Confirm Delete' dialogue so that, if possible
it shows the track and artist name rather than the raw file path.
I'm a little dissatisfied with how the two names aren't in tidy columns.
Would a complete overhaul to tidy this up be justified or plain overkill?
Oh, and I hope that no one minds me adding me adding my copy right
to the file, I'm not certain how much of a change is needed to justify
this.
diff --git a/src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp b/src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
index 349464c..a3ec56f 100644
--- a/src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
+++ b/src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
@@ -1,4 +1,5 @@
/****
+ * Copyright (c) 2011 Ryan McCoskrie *
* Copyright (c) 2010 Maximilian Kossick *
* Copyright (c) 2010 Casey Link *
* *
@@ -32,8 +33,23 @@ CollectionLocationDelegateImpl::reallyDelete( CollectionLocation *loc, const Met
Q_UNUSED( loc );
QStringList files;
-foreach( Meta::TrackPtr track, tracks )
-files << track->prettyUrl();
+foreach( Meta::TrackPtr track, tracks ){
+//Check what metadata is available before formating the string
+QString str;
+Meta::ArtistPtr artist = track->artist();
+if( ! track->prettyName().isEmpty()){ //Found track name
+str = track->prettyName();
+if( ! artist->prettyName().isEmpty() ) //... and artist name
+str += ( i18n(" by ") + artist->prettyName() );
+}
+else{ //Did not find track name
+str = track->prettyUrl();
+if( ! artist->prettyName().isEmpty()) //but did find artist name
+str += QString(" (%1)").arg(artist->prettyName());
+}
+
+files << str;
+}
// NOTE: taken from SqlCollection
// TODO put the delete confirmation code somewhere else?
@@ -54,8 +70,23 @@ CollectionLocationDelegateImpl::reallyTrash( CollectionLocation *loc, const Meta
Q_UNUSED( loc );
QStringList files;
-foreach( Meta::TrackPtr track, tracks )
-files << track->prettyUrl();
+foreach( Meta::TrackPtr track, tracks ){
+//Check what metadata is available before formating the string
+QString str;
+Meta::ArtistPtr artist = track->artist();
+if( ! track->prettyName().isEmpty()){ //Found track name
+str = track->prettyName();
+if( ! artist->prettyName().isEmpty() ) //... and artist name
+str += ( i18n(" by ") + artist->prettyName() );
+}
+else{ //Did not find track name
+str = track->prettyUrl();
+if( ! artist->prettyName().isEmpty()) //but did find artist name
+str += QString(" (%1)").arg(artist->prettyName());
+}
+
+files << str;
+}
const QString text( i18ncp( "@info",
"Do you really want to move this track to the trash? "
@@ -76,8 +107,23 @@ bool CollectionLocationDelegateImpl::reallyMove(CollectionLocation* loc, const M
{
Q_UNUSED( loc )
QStringList files;
-foreach( Meta::TrackPtr track, tracks )
-files << track->prettyUrl();
+foreach( Meta::TrackPtr track, tracks ){
+//Check what metadata is available before formating the string
+QString str;
+Meta::ArtistPtr artist = track->artist();
+if( ! track->prettyName().isEmpty()){ //Found track name
+str = track->prettyName();
+if( ! artist->prettyName().isEmpty() ) //... and artist name
+str += ( i18n(" by ") + artist->prettyName() );
+}
+else{ //Did not find track name
+str = track->prettyUrl();
+if( ! artist->prettyName().isEmpty()) //but did find artist name
+str += QString(" (%1)").arg(artist->prettyName());
+}
+
+files << str;
+}
const QString text( i18ncp( "@info", "Do you really want to move this track? It will be renamed and the original deleted.",
"Do you really want to move these %1 tracks? They will be renamed and the originals deleted.", tracks.count() ) );
@@ -92,8 +138,23 @@ void CollectionLocationDelegateImpl::errorDeleting( CollectionLocation* loc, con
{
Q_UNUSED( loc );
QStringList files;
-foreach( Meta::TrackPtr track, tracks )
-files << track->prettyUr
