ubuntu-touch-coreapps-reviewers team mailing list archive

[Roman Shchekin <mrqtros@xxxxxxxxx>]

Review: Approve

Left few comments in code. Overall feeling - ok.

Diff comments:

> 
> === added file 'src/app/qml/common/ContentHubProxy.qml'
> --- src/app/qml/common/ContentHubProxy.qml	1970-01-01 00:00:00 +0000
> +++ src/app/qml/common/ContentHubProxy.qml	2015-09-19 15:40:39 +0000
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2012-2014 Canonical, Ltd.
> + *
> + * 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 3.
> + *
> + * 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, see <http://www.gnu.org/licenses/>.
> + */
> +
> +import QtQuick 2.3
> +import Ubuntu.Content 1.1
> +import DocumentViewer 1.0
> +
> +// TODO: Show a dialog asking for the destination (internal storage or SD card)
> +
> +Item {
> +    id: contentHubProxy
> +
> +    property var activeTransfer
> +
> +    property alias rejectedDocuments: rejectedDocsModel
> +    property alias importedDocuments: importedDocsModel
> +
> +    property bool multipleSelection: activeTransfer ? (activeTransfer.selectionType !== ContentTransfer.Single) : true

Should be rewritten as:
    property bool multipleSelection: !activeTransfer || activeTransfer.selectionType !== ContentTransfer.Single
But looks strange anyway. Why do we allow multiple selection when transfer isn't acitve?

> +
> +    ListModel { id: rejectedDocsModel }
> +    ListModel { id: importedDocsModel }
> +
> +    ContentTransferHint {
> +        activeTransfer: contentHubProxy.activeTransfer
> +    }
> +
> +    Connections {
> +        target: ContentHub
> +
> +        onImportRequested: {
> +             activeTransfer = transfer;

We do not use semicolons, so it's better to remove here too.

> +
> +            if (activeTransfer.state === ContentTransfer.Charged) {
> +                mainView.switchToBrowseMode()
> +
> +                internal.clearModels()
> +
> +                for (var i=0; i<activeTransfer.items.length; i++) {
> +                    var sourcePath = internal.getPathFromUrl(activeTransfer.items[i].url)
> +
> +                    if (DocumentViewer.isFileSupported(sourcePath)) {
> +                        var documentsLocation = DocumentViewer.getXdgDocumentsLocation()
> +                        var destPath = DocumentViewer.buildDestinationPath(documentsLocation, sourcePath);
> +
> +                        internal.importDocument(sourcePath, destPath)
> +
> +                    } else {
> +                        // Document is not supported, append its entry into the
> +                        // rejected documents model, so that we can inform the
> +                        // user of what happened.
> +                        rejectedDocsModel.append({ path: sourcePath })
> +                    }
> +                }
> +
> +                internal.finalizeImport()
> +
> +                internal.handleNotifications()
> +            }
> +        }
> +
> +        onExportRequested: {
> +            activeTransfer = transfer;

And here.

> +            mainView.switchToPickMode()
> +        }
> +    }
> +
> +    QtObject {
> +        id: internal
> +
> +        function __openDocument() {
> +            if (contentHubProxy.importedDocuments.count > 1) {
> +                // If it has been imported more than a document, show
> +                // a file picker when user taps the "open" action.
> +                PopupUtils.open(
> +                            Qt.resolvedUrl("common/PickImportedDialog.qml"),
> +                            mainView,
> +                            {
> +                                parent: mainView,
> +                                model: contentHubProxy.importedDocuments
> +                            });
> +            } else {
> +                // It has been imported just a document, open it when
> +                // user taps the action button.
> +                mainView.openDocument(contentHubProxy.importedDocuments.get(0).path);
> +            }
> +        }
> +
> +        function clearModels() {
> +            rejectedDocsModel.clear()
> +            importedDocsModel.clear()
> +        }
> +
> +        function getPathFromUrl(url) {
> +            return url.toString().replace("file://", "")
> +        }
> +
> +        function importDocument(sourcePath, destPath) {
> +            DocumentViewer.copy(sourcePath, destPath);
> +            importedDocsModel.append({ path: destPath })
> +        }
> +
> +        function finalizeImport() {
> +            activeTransfer.finalize()
> +        }
> +
> +        function handleNotifications() {
> +            // Check if there's any rejected document in the last transfer.
> +            // If so, show an error dialog.
> +            if (contentHubProxy.rejectedDocuments.count > 0) {
> +                var rejectedDialog = PopupUtils.open(
> +                            Qt.resolvedUrl("common/RejectedImportDialog.qml"),
> +                            mainView,
> +                            {
> +                                parent: mainView,
> +                                model: contentHubProxy.rejectedDocuments
> +                            });
> +                rejectedDialog.closed.connect(openDocument)
> +            } else {
> +                // Open the document, or show a pick dialog if more than one have been imported.
> +                __openDocument()
> +            }
> +        }
> +    }
> +}


-- 
https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/10-reboot-contenthub-switch-to-qml-apis/+merge/271466
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app/reboot.