Fix unexpected behaviour when calling dolphin with a file as an argument
Needs RevisionPublic

Authored by krutovmikhail on Mar 25 2019, 6:12 PM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Dolphin
Summary

BUG: 377116

Test Plan

$ dolphin some/file some/directory

$ dolphin some/file some/other/file some/file_second
etc

Diff Detail

Repository
R318 Dolphin
Branch
mkrutov/20190325/feat/files_as_arguments
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10173
Build 10191: arc lint + arc unit
krutovmikhail created this revision.Mar 25 2019, 6:12 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 25 2019, 6:12 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
krutovmikhail requested review of this revision.Mar 25 2019, 6:12 PM
ngraham retitled this revision from BUG: 377116 Unexpected behaviour when calling dolphin with a file as an argument to Fix unexpected behaviour when calling dolphin with a file as an argument.Mar 25 2019, 7:30 PM
ngraham edited the summary of this revision. (Show Details)
elvisangelaccio requested changes to this revision.Mar 25 2019, 9:05 PM
elvisangelaccio added a subscriber: elvisangelaccio.

I get a crash if I run dolphin /etc/fstab:

[12762 - default] unknown(): ASSERT: "dirs.size() > 0" in file ../src/dolphintabwidget.cpp, line 175
This revision now requires changes to proceed.Mar 25 2019, 9:05 PM
krutovmikhail added a comment.EditedMar 25 2019, 9:24 PM

Thanks for noticing! I've updated the patch to fix the crash.

Btw,

$ dolphin --select

(without file argument)

Opens up /home with $HOME directory selected. Is it the expected behavior?

apol added a subscriber: apol.Mar 25 2019, 11:36 PM

I'm not an expert in dolphin but other than that it looks good to me. +1

Thanks!

src/main.cpp
142

No need to initialize with an empty list, it will be filled by default.
Just leave QList<QUrl> urlDirectories;

163–168

Use !urlDirectories.isEmpty(), it's a bit more direct.

pino added a subscriber: pino.Mar 26 2019, 5:32 AM
pino added inline comments.
src/main.cpp
146

toLocalFile() instead of path(), otherwise it breaks on Windows

  • Update per phabricator comments: Windows crash, intialization & logic
krutovmikhail marked 3 inline comments as done.Mar 26 2019, 8:52 PM

Thanks for your comments! Revision updated.

elvisangelaccio requested changes to this revision.Apr 7 2019, 7:55 PM
elvisangelaccio added inline comments.
src/main.cpp
144

Please make url a const reference. We should also prevent a possible detaching by using qAsConst(urls):

for (const QUrl &url : qAsConst(urls))
This revision now requires changes to proceed.Apr 7 2019, 7:55 PM