openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

1. There's a "csv" module, use it.
2. I should be able to specify the file name, not just where to save it to.
3. Have you tested this with > 1000 songs? How long does it take? Some sort of progress window necessary?
4. You call it a report internally, but you're not very specific for the user. Rather  call it a "Song List Report".

More comments inline.

Diff comments:

> === modified file 'openlp/core/common/settings.py'
> --- openlp/core/common/settings.py	2016-07-31 11:58:54 +0000
> +++ openlp/core/common/settings.py	2016-09-08 05:12:22 +0000
> @@ -379,6 +379,7 @@
>              'shortcuts/themeScreen': [QtGui.QKeySequence(QtCore.Qt.Key_T)],
>              'shortcuts/toolsReindexItem': [],
>              'shortcuts/toolsFindDuplicates': [],
> +            'shortcuts/ReportSongList': [],

Surely this should have the same naming convention as the other items?

>              'shortcuts/toolsAlertItem': [QtGui.QKeySequence(QtCore.Qt.Key_F7)],
>              'shortcuts/toolsFirstTimeWizard': [],
>              'shortcuts/toolsOpenDataFolder': [],
> 
> === modified file 'openlp/plugins/songs/songsplugin.py'
> --- openlp/plugins/songs/songsplugin.py	2016-03-31 16:34:22 +0000
> +++ openlp/plugins/songs/songsplugin.py	2016-09-08 05:12:22 +0000
> @@ -151,19 +152,37 @@
>          :param tools_menu: The actual **Tools** menu item, so that your actions can use it as their parent.
>          """
>          log.info('add tools menu')
> +        self.tools_menu = tools_menu
> +        self.song_tools_menu = QtWidgets.QMenu(tools_menu)
> +        self.song_tools_menu.setObjectName('song_tools_menu')
> +        self.song_tools_menu.setTitle(translate('SongsPlugin', 'Song Tools'))

The item is in the "Tools" menu, you don't need to say "Song Tools", just say "Songs"

>          self.tools_reindex_item = create_action(
>              tools_menu, 'toolsReindexItem',
>              text=translate('SongsPlugin', '&Re-index Songs'),
>              icon=':/plugins/plugin_songs.png',
>              statustip=translate('SongsPlugin', 'Re-index the songs database to improve searching and ordering.'),
> -            visible=False, triggers=self.on_tools_reindex_item_triggered)
> -        tools_menu.addAction(self.tools_reindex_item)
> +            triggers=self.on_tools_reindex_item_triggered)
>          self.tools_find_duplicates = create_action(
>              tools_menu, 'toolsFindDuplicates',
>              text=translate('SongsPlugin', 'Find &Duplicate Songs'),
>              statustip=translate('SongsPlugin', 'Find and remove duplicate songs in the song database.'),
> -            visible=False, triggers=self.on_tools_find_duplicates_triggered, can_shortcuts=True)
> -        tools_menu.addAction(self.tools_find_duplicates)
> +            triggers=self.on_tools_find_duplicates_triggered, can_shortcuts=True)
> +        self.tools_report_song_list = create_action(
> +            tools_menu, 'ReportSongList',
> +            text=translate('SongsPlugin', 'Export List of Songs'),

You call it a report internally, but you're not very specific for the user. Rather  call it a "Song List Report".

> +            statustip=translate('SongsPlugin', 'Produce a CSV file of all the songs in the database.'),
> +            triggers=self.on_tools_report_song_list_triggered)
> +
> +        self.tools_menu.addAction(self.song_tools_menu.menuAction())
> +        self.song_tools_menu.addAction(self.tools_reindex_item)
> +        self.song_tools_menu.addAction(self.tools_find_duplicates)
> +        self.song_tools_menu.addAction(self.tools_report_song_list)
> +
> +        self.song_tools_menu.menuAction().setVisible(False)
> +
> +    @staticmethod
> +    def on_tools_report_song_list_triggered():
> +        reporting.report_song_list()
>  
>      def on_tools_reindex_item_triggered(self):
>          """


-- 
https://code.launchpad.net/~trb143/openlp/reporting/+merge/305167
Your team OpenLP Core is subscribed to branch lp:openlp.