Details
- Reviewers
mwolff broulik hein - Group Reviewers
Plasma - Commits
- R119:de0e1bc156b7: Use QUrl in the ScreenMapper API
R119:cb1a2f8e70a0: Use QUrl in the ScreenMapper API
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
To be honest, I'm not sure the benefit of a nice API warrants the rest of the ugliness, as some parts still rely on using a string for URL (FolderModel::url) and makes the unit tests also uglier.
| containments/desktop/plugins/folder/screenmapper.cpp | ||
|---|---|---|
| 113 | "== QLatin1Char('/') ?" | |
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 1062 | move down into the first conditional, return original url when nothing changed | |
| 1068 | return QUrl::fromUserInput(mappedUrl, {}, QUrl::AssumeLocalFile) | |
| 1072 | return url; | |
| 1510–1511 | 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 everywhere inplace of the QUrl::fromUserInput three-arg function call | |
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.
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 291 | couldn't you use this->resolvedUrl() here? | |
| 327 | shouldn't/couldn't this use resolvedUrl? | |
| 633 | resolvedUrl()? | |
Change code according to reviewer's requests
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 1510–1511 | m_url is exposed to QML, that doesn't support QUrl. | |