D9325: Use QUrl in the ScreenMapper API

amantia closed this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D9325 To: amantia, #plasma, mwolff, broulik, hein Cc: ervin, mlaurent, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D9325: Use QUrl in the ScreenMapper API

amantia updated this revision to Diff 25933. amantia added a comment. Make the code more clear, by renaming a local variable. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9325?vs=25799&id=25933 BRANCH master REVISION DETAIL https://phabricat

D9325: Use QUrl in the ScreenMapper API

mwolff accepted this revision. mwolff added a comment. This revision is now accepted and ready to land. two questions, otherwise lgtm INLINE COMMENTS > foldermodel.cpp:291 > > -const auto oldUrl = m_url; > +const auto oldUrl = this->resolvedUrl(); > could you call this at the top

D9325: Use QUrl in the ScreenMapper API

hein accepted this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D9325 To: amantia, #plasma, mwolff, broulik, hein Cc: ervin, mlaurent, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D9325: Use QUrl in the ScreenMapper API

amantia edited reviewers, added: hein; removed: dakon. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D9325 To: amantia, #plasma, mwolff, broulik, hein Cc: ervin, mlaurent, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas,

D9325: Use QUrl in the ScreenMapper API

amantia added inline comments. INLINE COMMENTS > mwolff wrote in foldermodel.cpp:1509 > not your change: why is m_url not an url :-/ > > also: introduce the helper function you have in the tests here, too - maybe > even move it into a static function in the ScreenMapper and then use it > every

D9325: Use QUrl in the ScreenMapper API

amantia updated this revision to Diff 25799. amantia added a comment. Change code according to reviewer's requests REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9325?vs=23954&id=25799 BRANCH master REVISION DETAIL https://phabricator.kde.org/D

D9325: Use QUrl in the ScreenMapper API

mwolff added a comment. I'd personally prefer to use an API with less conversions if possible. The fact that the test code becomes (marginally) more complex doesn't really count in my opinion. INLINE COMMENTS > foldermodel.cpp:291 > > -const auto oldUrl = m_url; > +const auto oldU

D9325: Use QUrl in the ScreenMapper API

ervin added a comment. @mwolff and @broulik Could one of you answer @amantia concerns regarding the motivations of that patch? If you agree with him maybe we don't want that patch at all... REVISION DETAIL https://phabricator.kde.org/D9325 To: amantia, #plasma, mwolff, dakon, broulik Cc:

D9325: Use QUrl in the ScreenMapper API

mwolff requested changes to this revision. mwolff added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > foldermodel.cpp:1063 > +auto mappableUrl = [this, dropTargetFolderUrl](const QUrl &url) -> QUrl { > QString mappedUrl = url.toString(); >

D9325: Use QUrl in the ScreenMapper API

mlaurent added inline comments. INLINE COMMENTS > screenmapper.cpp:101 > const auto screenPathWithScheme = screenUrl.url(); > -const bool isEmpty = (path.isEmpty() || screenUrl.path() == "/"); > +const bool isEmpty = (screenUrl.isEmpty() || screenUrl.path() == "/"); > // restore

D9325: Use QUrl in the ScreenMapper API

amantia updated this revision to Diff 23954. amantia added a comment. Fixed issues pointed out by Laurent. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9325?vs=23898&id=23954 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9325 AFFECTED FILES containments/deskto

D9325: Use QUrl in the ScreenMapper API

mlaurent added inline comments. INLINE COMMENTS > foldermodeltest.cpp:34 > > +static QUrl stringToUrl(const QString &path) { > +return QUrl::fromUserInput(path, {}, QUrl::AssumeLocalFile); new line after ')' > positionertest.cpp:38 > > +static QUrl stringToUrl(const QString &path) { > +

D9325: Use QUrl in the ScreenMapper API

amantia added reviewers: Plasma, mwolff, dakon, broulik. amantia set the repository for this revision to R119 Plasma Desktop. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabric