This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:78329e2eb60d: KDirModel: implement showing a root node
for the requested URL (authored by dfaure).
CHANGED PRIOR TO
rrosch added a comment.
Tested, no crashes, it works as intended, and it is beautiful! Thank you!
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D25315
To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, ahmadsamir, kde-frameworks-devel, LeGast00n,
rrosch added a comment.
Hi, sorry for the lack of responses, for some reason I did not receive any of
the notifications except this last one. I will test and comment as soon as I
can. Wishing everyone safety and health too.
REPOSITORY
R241 KIO
REVISION DETAIL
dfaure added a subscriber: rrosch.
dfaure added a comment.
@rrosch Does this now behave as expected?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D25315
To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, ahmadsamir, kde-frameworks-devel, LeGast00n,
ahmadsamir added inline comments.
INLINE COMMENTS
> dfaure wrote in kdirmodeltest_gui.cpp:92
> It doesn't look "normal" : without the ShowRoot feature, you wouldn't see the
> "/" root node.
> So I would say the test program is valid, no matter what the starting
> directory is.
>
> (I was
dfaure added inline comments.
INLINE COMMENTS
> ahmadsamir wrote in kdirmodeltest_gui.cpp:92
> I did test the /usr/share/fonts path :); it's just that starting at "/"
> looked "normal", whereas starting at a specific dir conveyed the goal of this
> change better, to me anyway.
It doesn't look
ahmadsamir added inline comments.
INLINE COMMENTS
> dfaure wrote in kdirmodel.h:79
> That sounds more confusing to me, depending on how one thinks about all this.
>
> There's nothing special about the first child compared to other direct
> children, one misinterpretation of your suggested
dfaure marked an inline comment as done.
dfaure added inline comments.
INLINE COMMENTS
> ahmadsamir wrote in kdirmodel.h:79
> I suggest:
> s/its children/the first child/ OR
> s/at its children/directly at the first child/
That sounds more confusing to me, depending on how one thinks about all
dfaure updated this revision to Diff 77224.
dfaure added a comment.
Add missing braces around one-line if()
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25315?vs=77155=77224
BRANCH
2019_11_kdirmodel_root
REVISION DETAIL
ahmadsamir added inline comments.
INLINE COMMENTS
> kdirmodeltest_gui.cpp:92
> +dirmodel->openUrl(QUrl(QStringLiteral("file:///")),
> KDirModel::ShowRoot);
>
> const QUrl url =
> QUrl::fromLocalFile(QStringLiteral("/usr/share/applications/kde"));
I think a test with
ahmadsamir added inline comments.
INLINE COMMENTS
> kdirmodel.cpp:222
> +QUrl
> parent(url.adjusted(QUrl::RemoveFilename|QUrl::StripTrailingSlash));
> +if (url.path() == QLatin1String("/"))
> +parent.setPath(QString());
Braces around if.
> kdirmodel.h:79
> + *
dfaure updated this revision to Diff 77155.
dfaure added a comment.
Strip trailing slashes, add test
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25315?vs=76701=77155
BRANCH
2019_11_kdirmodel_root
REVISION DETAIL
https://phabricator.kde.org/D25315
dfaure added a comment.
OK, reproduced by adding a test for a URL with a trailing slash. Fix coming
up.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D25315
To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2,
dfaure added a comment.
Well, I don't have your code. Which calls are you making into KDirModel?
(openUrl(args=?), expandToUrl(args=?))
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D25315
To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc:
rrosch added a comment.
Crash:
kf5.kio.kdirmodel: Items emitted in directory QUrl("file:///home/myuser/")
but that directory isn't in KDirModel! Root directory:
QUrl("file:///home/myuser/")
KCrash: Application 'konqueror' crashing...
This happens right after pressing F9 to
dfaure updated this revision to Diff 76701.
dfaure retitled this revision from "KDirModel: implement showing "/" as a root
node, optionally" to "KDirModel: implement showing a root node for the
requested URL".
dfaure edited the summary of this revision.
dfaure removed a subscriber: rrosch.
16 matches
Mail list logo
