ubuntu-touch-coreapps-reviewers team mailing list archive

[Roman Shchekin <mrqtros@xxxxxxxxx>]

Review: Needs Fixing

See inline comments.

Diff comments:

> 
> === modified file 'shorts/qml/components/NetworkManager.qml'
> --- shorts/qml/components/NetworkManager.qml	2015-10-24 07:06:24 +0000
> +++ shorts/qml/components/NetworkManager.qml	2015-12-15 16:05:21 +0000
> @@ -129,4 +142,133 @@
>              }
>          } // GFA
>      } // QtObject
> +

A lot of code is duplicated here, we should follow 'Don't repeat yourself' rule and generialize this code.

> +    //////////////////////////////////////////  add a new object to refresh non-google feeds
> +    QtObject {
> +        id: dNG
> +
> +        property var feedList: []                 // Feed list to update.
> +        property var currentFeed                  // Current downloading feed.
> +        property int tagId: 0                     // Tag to update.
> +
> +        /* Method updates feeds one by another.
> +             * Input: array of objects, each should include
> +             * source, link and id (of feed in DB) properties.
> +             */
> +        function updateFeeds(feedsArray, topicId) {
> +            tagId = topicId || 0
> +
> +            downloadStarted(tagId)
> +
> +            feedList = feedsArray
> +            operationStatus = "success"
> +            updateNextFeed()
> +        }
> +
> +        // For inner usage only.
> +        function updateNextFeed() {
> +            if (feedList.length == 0) {
> +                downloadFinished(tagId)
> +                return
> +            }
> +
> +            currentFeed = feedList.shift()
> +            nonGoogleFeedApi.loadFeed(currentFeed.source)
> +        }
> +
> +        function cancelDownload() {
> +            feedList = []
> +            operationStatus = "abort"
> +            nonGoogleFeedApi.abort()
> +        }
> +
> +        function updateFeedInfo(feedId, feedLink, responseData) {
> +            var entries = responseData.item
> +            var f = responseData
> +
> +            var fde = f.description == undefined ? "" : f.description["#text"] == undefined ? f.description : f.description["#text"]
> +            var fti = f.title == undefined ? "" : f.title["#text"] == undefined ? f.title : f.title["#text"]
> +
> +            DB.updateFeedByXml(feedId, feedLink, fde, fti)
> +            console.log(" -------- UPDATE INFO -------- ")
> +            //            console.log(f.title, f.link, f.feedUrl, f.description)
> +
> +            console.time("addArticlesEx")
> +
> +            var newArticles = []
> +            var maxLength = entries.length > 50 ? 50 : entries.length
> +            for (var i = 0; i < maxLength; i++) {
> +                var e = entries[i]
> +
> +                //                print("article detail: ", JSON.stringify(e))
> +                // Grab image from for article.
> +                //                var articleImage = ImageUtils.grabArticleImage(e)
> +                //                e.content = clearFromBadTags(e.content)
> +
> +                var ti = e.title == undefined ? "" : e.title["#text"] == undefined ? e.title : e.title["#text"]
> +                var li = e.link == undefined ? "" : e.link["#text"] == undefined ? e.link : e.link["#text"]
> +                var au = e.author == undefined ? "" : e.author["#text"] == undefined ? e.author : e.author["#text"]
> +                var creator = e.creator == undefined ? "" : e.creator["#text"] == undefined ? e.creator : e.creator["#text"]
> +                var de = e.description == undefined ? "" : e.description["#text"] == undefined ? e.description : e.description["#text"]
> +                var pu = e.pubDate == undefined ? "" : e.pubDate["#text"] == undefined ? e.pubDate : e.pubDate["#text"]
> +                var co = e.content == undefined ? "" : e.content["#text"] == undefined ? e.content : e.content["#text"]
> +                var articleImage = utilities.htmlGetImg(de)
> +                if (articleImage.length == 0) { articleImage = utilities.htmlGetImg(co) }
> +                else {
> +                    print("articleImage: ", articleImage[0])
> +                }
> +                //                print("date parse 0: ", pu, DateUtils.parseDate(pu))
> +                //                print("date parse 1: ", DateUtils.formatRelativeTime(i18n, DateUtils.parseDate(pu)))
> +
> +                var temp =
> +                        {
> +                    "title": ti,
> +                    "content": co == "" ? de : co,
> +                                          "link": li,
> +                                          "author": creator == "" ? au : creator,
> +                                                                    "description": de,
> +                                                                    "pubDate": DateUtils.parseDate(pu),
> +                                                                    "guid": Qt.md5(li + pu),
> +                                                                    "image" : articleImage.length > 0 ? articleImage[0] : "",
> +                                                                                                        "media_groups" : ""
> +                }
> +
> +                newArticles.push(temp)
> +            }
> +//            print("new article length: ", newArticles.length)
> +
> +            //            /* Add new articles to DB and restore 'read' status of some of them.
> +            //             */
> +            //            DB.addArticles(articleModel, feedId, articleProperties);
> +
> +            /* Add new articles to DB and restore 'read' status of some of them. */
> +            try {
> +                DB.addArticlesEx(newArticles, feedId)
> +            } catch (e) {
> +                console.log("Exception:", JSON.stringify(e))
> +            }
> +
> +            console.timeEnd("addArticlesEx")
> +        }
> +
> +        function clearFromBadTags(content) {
> +            /* Remove non empty too. Useless anyway.
> +                 */
> +            content = content.replace(/alt=".*?"/g, "")
> +            content = content.replace(/title=".*?"/g, "")
> +            return content
> +        }
> +
> +        property var nonGoogleFeedApi: XmlNetwork {
> +            onLoadResult: {
> +                if (result.rss == undefined || result.rss == "") {
> +//                    console.log("XML NETWORK GFA:", JSON.stringify(result))
> +                    if (operationStatus == "success")
> +                        operationStatus = "withErrors"
> +                } else dNG.updateFeedInfo(dNG.currentFeed.id, dNG.currentFeed.link, result.rss.channel)
> +
> +                dNG.updateNextFeed()
> +            }
> +        } // GFA
> +    } // QtObject
>  }
> 
> === added file 'shorts/qml/nongoogle/XmlNetwork.qml'
> --- shorts/qml/nongoogle/XmlNetwork.qml	1970-01-01 00:00:00 +0000
> +++ shorts/qml/nongoogle/XmlNetwork.qml	2015-12-15 16:05:21 +0000
> @@ -0,0 +1,84 @@
> +import QtQuick 2.4
> +
> +QtObject {
> +    id: rootObject
> +
> +    property bool inProgress: __doc != null
> +
> +    signal findResult(var result)
> +    signal loadResult(var result)
> +
> +    property var __doc: null
> +
> +    /* Load feed by URL.
> +     */
> +    function loadFeed(feedUrl, num) {
> +        abort(true)
> +
> +        if (num)
> +            num = Math.min(num, 100)
> +        else num = 50
> +
> +        __doc = new XMLHttpRequest()
> +        var doc = __doc
> +
> +        doc.onreadystatechange = function() {
> +
> +//            print("xmlnetwork onreadystatechange: ", doc.readyState, doc.status, feedUrl)
> +            if (doc.readyState === XMLHttpRequest.DONE) {
> +
> +                var resObj
> +                if (doc.status == 200) {
> +//                    resObj = JSON.parse(doc.responseText)
> +                    resObj = utilities.xmlToJson(doc.responseText)
> +                } else { // Error
> +                    resObj = {"responseDetails" : doc.statusText,
> +                        "responseStatus" : doc.status}
> +                }
> +
> +                __doc = null
> +                loadResult(resObj)
> +            }
> +        }
> +
> +
> +        /* Number of articles to download.

These comments are not actual for your code, only for GFA.

> +         */
> +//        finalRequest += "&num=" + num
> +
> +        /* Add some optional params.
> +         * May be usable:
> +         * hl       - host language, for example "hl=ru", default en.
> +         * num      - number of entries, for example "num=50", default 4, maximum 100.
> +         * output   - format of output, for example "output=json", may be xml, json_xml, json.
> +         */
> +//        doc.open("GET", finalRequest, true);
> +        doc.open("GET", feedUrl, true);
> +        doc.send();
> +    }
> +
> +    /* Param "isAbortOnly" used to preserve
> +     * property "__doc" in not null state.
> +     * inProgress binded to it so we can avoid
> +     * additional recalculations.
> +     */
> +    function abort(isAbortOnly) {
> +        if (__doc != null) {
> +            __doc.abort()
> +            if (!isAbortOnly)
> +                __doc = null
> +        }
> +    }
> +
> +    /* Return true if some kind of errors detected.
> +     * TODO DEMO
> +     */
> +    function checkForErrors(result) {
> +        if (result.responseStatus == 200 ||   // HTTP OK
> +                result.responseStatus == 0) { // ABORTED
> +            return false
> +        }
> +
> +        return true
> +    }
> +} // QtObject
> 
> === added file 'shorts/qml/pages/PageSettings.qml'

Can I ask you to rename it to 'SettingsPage'?



-- 
https://code.launchpad.net/~qqworini/ubuntu-rssreader-app/add-non-google-support/+merge/280614
Your team Ubuntu Shorts Developers is subscribed to branch lp:ubuntu-rssreader-app.