openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

I've added some inline comments for some things I think need to be fixed up. Mostly small stuff.

Diff comments:

> 
> === added file 'openlp/core/ui/lib/filedialogpatches.py'
> --- openlp/core/ui/lib/filedialogpatches.py	1970-01-01 00:00:00 +0000
> +++ openlp/core/ui/lib/filedialogpatches.py	2017-08-04 22:00:43 +0000
> @@ -0,0 +1,118 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2017 OpenLP Developers                                   #
> +# --------------------------------------------------------------------------- #
> +# This program is free software; you can redistribute it and/or modify it     #
> +# under the terms of the GNU General Public License as published by the Free  #
> +# Software Foundation; version 2 of the License.                              #
> +#                                                                             #
> +# This program is distributed in the hope that it will be useful, but WITHOUT #
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or       #
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for    #
> +# more details.                                                               #
> +#                                                                             #
> +# You should have received a copy of the GNU General Public License along     #
> +# with this program; if not, write to the Free Software Foundation, Inc., 59  #
> +# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
> +###############################################################################
> +""" Patch the QFileDialog so it accepts and returns Path objects"""
> +from functools import wraps
> +from pathlib import Path
> +
> +from PyQt5 import QtWidgets
> +
> +from openlp.core.common.path import path_to_str, str_to_path
> +from openlp.core.lib import replace_params
> +
> +
> +class PQFileDialog(QtWidgets.QFileDialog):

Can we just call this, "FileDialog"? You said you're replacing the other dialog, let's leave off the PQ, we don't need those prefixes in our code.

> +    @classmethod
> +    @wraps(QtWidgets.QFileDialog.getExistingDirectory)
> +    def getExistingDirectory(cls, *args, **kwargs):
> +        """
> +        Reimplement `getExistingDirectory` so that it can be called with, and return Path objects
> +
> +        :type parent: QtWidgets.QWidget or None
> +        :type caption: str
> +        :type directory: pathlib.Path
> +        :type options: QtWidgets.QFileDialog.Options
> +        :rtype: tuple[Path, str]
> +        """
> +
> +        args, kwargs = replace_params(args, kwargs, ((2, 'directory', path_to_str),))
> +
> +        return_value = super().getExistingDirectory(*args, **kwargs)
> +
> +        # getExistingDirectory returns a str that represents the path. The string is empty if the user cancels the
> +        # dialog.
> +        return str_to_path(return_value)
> +
> +    @classmethod
> +    @wraps(QtWidgets.QFileDialog.getOpenFileName)
> +    def getOpenFileName(cls, *args, **kwargs):
> +        """
> +        Reimplement `getOpenFileName` so that it can be called with, and return Path objects
> +
> +        :type parent: QtWidgets.QWidget or None
> +        :type caption: str
> +        :type directory: pathlib.Path
> +        :type filter: str
> +        :type initialFilter: str
> +        :type options: QtWidgets.QFileDialog.Options
> +        :rtype: tuple[Path, str]
> +        """
> +
> +        args, kwargs = replace_params(args, kwargs, ((2, 'directory', path_to_str),))
> +
> +        return_value = super().getOpenFileName(*args, **kwargs)

filename, is_ok = super().getOpenFileName(*args, **kwargs)

> +
> +        # getOpenFileName returns a tuple. The first item is a of str's that represents the path. The string is empty if
> +        # the user cancels the dialog.
> +        return str_to_path(return_value[0]), return_value[1]

return str_to_path(filename), is_ok

> +
> +    @classmethod
> +    def getOpenFileNames(cls, *args, **kwargs):
> +        """
> +        Reimplement `getOpenFileNames` so that it can be called with, and return Path objects
> +
> +        :type parent: QtWidgets.QWidget or None
> +        :type caption: str
> +        :type directory: pathlib.Path
> +        :type filter: str
> +        :type initialFilter: str
> +        :type options: QtWidgets.QFileDialog.Options
> +        :rtype: tuple[list[Path], str]
> +        """
> +        args, kwargs = replace_params(args, kwargs, ((2, 'directory', path_to_str),))
> +
> +        return_value = super().getOpenFileNames(*args, **kwargs)

paths, is_ok = super().getOpenFileName(*args, **kwargs)

> +
> +        # getSaveFileName returns a tuple. The first item is a list of str's that represents the path. The list is
> +        # empty if the user cancels the dialog.
> +        paths = [str_to_path(path) for path in return_value[0]]

paths = [str_to_path(path) for path in paths]

> +        return paths, return_value[1]
> +
> +    @classmethod
> +    def getSaveFileName(cls, *args, **kwargs):
> +        """
> +        Reimplement `getSaveFileName` so that it can be called with, and return Path objects
> +
> +        :type parent: QtWidgets.QWidget or None
> +        :type caption: str
> +        :type directory: pathlib.Path
> +        :type filter: str
> +        :type initialFilter: str
> +        :type options: QtWidgets.QFileDialog.Options
> +        :rtype: tuple[Path or None, str]
> +        """
> +        args, kwargs = replace_params(args, kwargs, ((2, 'directory', path_to_str),))
> +
> +        return_value = super().getSaveFileName(*args, **kwargs)

filename, is_ok = super().getSaveFileName(*args, **kwargs)

> +
> +        # getSaveFileName returns a tuple. The first item represents the path as a str. The string is empty if the user
> +        # cancels the dialog.
> +        return str_to_path(return_value[0]), return_value[1]

return str_to_path(filename), is_ok

> 
> === modified file 'openlp/core/ui/lib/pathedit.py' (properties changed: +x to -x)
> --- openlp/core/ui/lib/pathedit.py	2017-07-04 23:13:51 +0000
> +++ openlp/core/ui/lib/pathedit.py	2017-08-04 22:00:43 +0000
> @@ -38,11 +40,12 @@
>      The :class:`~openlp.core.ui.lib.pathedit.PathEdit` class subclasses QWidget to create a custom widget for use when
>      a file or directory needs to be selected.
>      """
> -    pathChanged = QtCore.pyqtSignal(str)
> +

Don't need this open line.

> +    pathChanged = QtCore.pyqtSignal(Path)
>  
>      def __init__(self, parent=None, path_type=PathType.Files, default_path=None, dialog_caption=None, show_revert=True):
>          """
> -        Initalise the PathEdit widget
> +        Initialise the PathEdit widget
>  
>          :param parent: The parent of the widget. This is just passed to the super method.
>          :type parent: QWidget or None
> 
> === modified file 'openlp/core/ui/themeform.py'
> --- openlp/core/ui/themeform.py	2017-05-30 18:50:39 +0000
> +++ openlp/core/ui/themeform.py	2017-08-04 22:00:43 +0000
> @@ -28,12 +28,14 @@
>  from PyQt5 import QtCore, QtGui, QtWidgets
>  
>  from openlp.core.common import Registry, RegistryProperties, UiStrings, translate, get_images_filter, is_not_image_file
> +from openlp.core.common.path import path_to_str, str_to_path
>  from openlp.core.lib.theme import BackgroundType, BackgroundGradientType
>  from openlp.core.lib.ui import critical_error_message_box
>  from openlp.core.ui import ThemeLayoutForm
>  from openlp.core.ui.media.webkitplayer import VIDEO_EXT
>  from .themewizard import Ui_ThemeWizard
>  
> +

Don't need this open line.

>  log = logging.getLogger(__name__)
>  
>  
> 
> === modified file 'openlp/core/ui/themewizard.py'
> --- openlp/core/ui/themewizard.py	2017-05-22 18:22:43 +0000
> +++ openlp/core/ui/themewizard.py	2017-08-04 22:00:43 +0000
> @@ -30,6 +30,8 @@
>  from openlp.core.lib.ui import add_welcome_page, create_valign_selection_widgets
>  from openlp.core.ui.lib import ColorButton, PathEdit
>  
> +from pathlib import Path

Isn't pathlib part of the Python standard library? If it is, its import belongs at the top, not the bottom. https://wiki.openlp.org/Development:Coding_Standards#Import_Order

> +
>  
>  class Ui_ThemeWizard(object):
>      """


-- 
https://code.launchpad.net/~phill-ridout/openlp/pathlib2/+merge/328616
Your team OpenLP Core is subscribed to branch lp:openlp.