Review Request 113390: GSoC 2013 Revamping Scripting - Part 6/6 : Misc src/

[Anmol Ahuja]

> On Nov. 16, 2013, 7:55 p.m., Mark Kretschmann wrote:
> > src/amarokconfig.kcfg, line 596
> > <http://git.reviewboard.kde.org/r/113390/diff/1/?file=204540#file204540line596>
> >
> >     The user probably does not want to see meaningless errors about some script they installed. This is a debugging feature for developers, right?

I intended it to be user facing, asking them to email the script writer, but that seemed like a pretty bad idea, so I've kept it disabled for now. I was thinking of enabling it whenever the script console is enabled.


> On Nov. 16, 2013, 7:55 p.m., Mark Kretschmann wrote:
> > src/configdialog/dialogs/ScriptsConfig.cpp, line 260
> > <http://git.reviewboard.kde.org/r/113390/diff/1/?file=204552#file204552line260>
> >
> >     Please reword the string. It's pretty unclear.
> >     
> >     E.g. "Default scripts that are bundled with Amarok cannot be uninstalled."

Because I'm not sure if this is a good idea, removing this could remove multiple scripts if they're nested in one folder in the scripts dir. But it shouldn't really happen unless the user manually copies them into the folder like that.


> On Nov. 16, 2013, 7:55 p.m., Mark Kretschmann wrote:
> > src/configdialog/dialogs/ScriptsConfig.cpp, line 289
> > <http://git.reviewboard.kde.org/r/113390/diff/1/?file=204552#file204552line289>
> >
> >     Can be changed to:
> >     
> >     return findSpecFile( subdir );
> >

No, it would then break out of the loop.


> On Nov. 16, 2013, 7:55 p.m., Mark Kretschmann wrote:
> > src/configdialog/dialogs/ScriptsConfig.cpp, line 255
> > <http://git.reviewboard.kde.org/r/113390/diff/1/?file=204552#file204552line255>
> >
> >     Don't use exclamation marks in user-visible strings.
> >     
> >     Also I'm wondering why an error dialog is shown here. Maybe the button should simply be disabled if nothing is selected. Then there would be no need for error handling.

Okay.


> On Nov. 16, 2013, 7:55 p.m., Mark Kretschmann wrote:
> > src/CMakeLists.txt, line 484
> > <http://git.reviewboard.kde.org/r/113390/diff/1/?file=204537#file204537line484>
> >
> >     So where should it be? :)

I'm not sure.
The script is generating autocomplete strings for the script console, which could remain in cmake, and also generating a pseudo header for doxygen, which should probably be done on the server where doxygen is run. I'll separate em.


- Anmol


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


On Oct. 22, 2013, 9:36 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113390/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 9:36 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> -------
> 
> Miscellaneous changes made in the src/ folder.
> 
> 
> Diffs
> -----
> 
>   src/App.h 9ee071d 
>   src/App.cpp eb3f483 
>   src/CMakeLists.txt 70fb67b 
>   src/EngineController.h 80ec17b 
>   src/EngineController.cpp cf29cf8 
>   src/amarokconfig.kcfg 84cfa5b 
>   src/amarokurls/BookmarkGroup.h ea42c30 
>   src/browsers/CollectionTreeItem.h 0a5d197 
>   src/browsers/CollectionTreeItem.cpp 4c35523 
>   src/browsers/CollectionTreeItemModelBase.h 00e99fb 
>   src/browsers/CollectionTreeView.h 454a70d 
>   src/browsers/CollectionTreeView.cpp 6fad164 
>   src/browsers/collectionbrowser/CollectionWidget.h 7480015 
>   src/browsers/collectionbrowser/CollectionWidget.cpp 95a16dc 
>   src/configdialog/dialogs/ScriptSelector.h 9ce1168 
>   src/configdialog/dialogs/ScriptSelector.cpp 117cedf 
>   src/configdialog/dialogs/ScriptsConfig.h 6a47847 
>   src/configdialog/dialogs/ScriptsConfig.cpp 714b63e 
>   src/configdialog/dialogs/ScriptsConfig.ui b4b8d37 
>   src/context/applets/lyrics/LyricsApplet.cpp 7db356d 
>   src/context/applets/songkick/SongkickApplet.cpp ed93e56 
>   src/context/engines/lyrics/LyricsEngine.cpp d3273b0 
>   src/core-impl/collections/support/jobs/WriteTagsJob.h a92ab8d 
>   src/core-impl/collections/support/jobs/WriteTagsJob.cpp 5834cd4 
>   src/core/collections/Collection.h 94d24d7 
>   src/core/collections/QueryMaker.h 92b6a65 
>   src/core/meta/support/MetaConstants.cpp 40d7122 
>   src/dialogs/DiagnosticDialog.cpp c9bfce6 
>   src/dialogs/EqualizerDialog.h 6c9e19e 
>   src/dialogs/EqualizerDialog.cpp acf0da0 
>   src/dialogs/deviceconfiguredialog.cpp e54edad 
>   src/dynamic/BiasFactory.cpp 2a887c2 
>   src/dynamic/TrackSet.h dee8ee3 
>   src/dynamic/TrackSet.cpp 456a10a 
>   src/equalizer/EqualizerPresets.h be8d267 
>   src/equalizer/EqualizerPresets.cpp b9aa13b 
>   src/playback/EqualizerController.h PRE-CREATION 
>   src/playback/EqualizerController.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h a835c35 
>   src/services/scriptable/ScriptableService.cpp f254e3e 
>   src/services/scriptable/ScriptableServiceInfoParser.cpp 998135b 
>   src/services/scriptable/ScriptableServiceManager.cpp aa28a6c 
>   src/services/scriptable/ScriptableServiceQueryMaker.cpp 07233dd 
> 
> Diff: http://git.reviewboard.kde.org/r/113390/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20131116/7a7b7a3a/attachment-0001.html>