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
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=25933
BRANCH
master
REVISION DETAIL
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
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
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,
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
>
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=25799
BRANCH
master
REVISION DETAIL
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
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:
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 ) -> QUrl {
> QString mappedUrl = url.toString();
> if
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() == "/");
> //
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=23954
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D9325
AFFECTED FILES
mlaurent added inline comments.
INLINE COMMENTS
> foldermodeltest.cpp:34
>
> +static QUrl stringToUrl(const QString ) {
> +return QUrl::fromUserInput(path, {}, QUrl::AssumeLocalFile);
new line after ')'
> positionertest.cpp:38
>
> +static QUrl stringToUrl(const QString ) {
> +
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
14 matches
Mail list logo
