This revision was automatically updated to reflect the committed changes.
Closed by commit R824:a81ceaeaa963: baloo-widgets: Refactor
filemetadataprovider for better readability (authored by michaelh).
REPOSITORY
R824 Baloo Widgets
CHANGES SINCE LAST UPDATE
ngraham added a comment.
I would have deleted the existing `arcpatch-D10105` branch so ad to avoid
`arc` creating a `arcpatch-D10105_1` branch, but yeah, that looks sane.
REPOSITORY
R824 Baloo Widgets
BRANCH
requestfinished (branched from master)
REVISION DETAIL
michaelh added a comment.
Just to be sure:
$ git checkout -b otto origin/master
$ arc patch D10105
now in branch arcpatch-D10105_1
$ arc checkout dataavail
$ arc rebase arcpatch-D10105_1
$ arc checkout unittest
$ arc rebase dataavail
and so on?
ngraham added a comment.
Yes, Arc tracks dependencies on the revision level, not the branch level.
If you're nervous, you don't even need to delete the local branch, either.
Just switch to master and pull down this copy of it with `arc patch D10105`
REPOSITORY
R824 Baloo Widgets
michaelh added a comment.
Are you aware of the 3 branches dependent on this one?
REPOSITORY
R824 Baloo Widgets
BRANCH
requestfinished (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin,
ngraham added a comment.
In cases like that, I usually delete the local branch and re-download the
patch using Arc's version: `arc patch D10105`. Then go onto the
`arcpatch-D10105` branch and `arc land --preview` from there.
REPOSITORY
R824 Baloo Widgets
BRANCH
requestfinished
michaelh added a comment.
Can't land this
Usage Exception: arc can not identify which revision exists on branch
'refactor'.
$ arc branch
arcpatch-D10105 Accepted D10105: baloo-widgets: Refactor
filemetadataprovider for better readability
master
elvisangelaccio accepted this revision as: elvisangelaccio.
elvisangelaccio added inline comments.
This revision is now accepted and ready to land.
INLINE COMMENTS
> filemetadataprovider.cpp:106
>
> +
> }
unrelated whitespace change
> filemetadataprovider.h:119
>
> +
> +private:
michaelh updated this revision to Diff 26134.
michaelh added a comment.
Correct typo
REPOSITORY
R824 Baloo Widgets
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10105?vs=26132=26134
BRANCH
requestfinished (branched from master)
REVISION DETAIL
michaelh added a comment.
@elvisangelaccio: Sorry for this updating mess. These nested branches are
just too error-prone for me, because they look so similar. Won't do that again.
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh,
michaelh updated this revision to Diff 26132.
michaelh marked an inline comment as done.
michaelh added a comment.
Add Q_ASSERT
REPOSITORY
R824 Baloo Widgets
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10105?vs=26079=26132
BRANCH
requestfinished (branched from master)
elvisangelaccio added inline comments.
INLINE COMMENTS
> michaelh wrote in filemetadataprovider.cpp:141
> I've been thinking about this...
>
> We already have guards against files.isEmpty() in
>
> 355if (!urls.isEmpty()) {
>
> and
>
> 373 } else if (items.size() == 1) {
>
> Unless
michaelh marked 2 inline comments as done.
michaelh added inline comments.
INLINE COMMENTS
> elvisangelaccio wrote in filemetadataprovider.cpp:141
> Why not? `insertEditableData()` just sets some defaults, how is it related to
> `files`?
I've been thinking about this...
We already have guards
elvisangelaccio added inline comments.
INLINE COMMENTS
> filemetadataprovider.cpp:137
> +
> +if (files.size() == 0) {
> +emit loadingFinished();
`files.isEmpty()`
> michaelh wrote in filemetadataprovider.cpp:141
> We can leave early here, I think.
> Without a file
>
> 149
michaelh edited the summary of this revision.
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin, #frameworks
michaelh marked 4 inline comments as done.
michaelh added inline comments.
INLINE COMMENTS
> filemetadataprovider.cpp:141
> }
> -else {
> -//
> -// Only report the stuff that is common to all the files
> -//
> -QSet allProperties;
> -QList
michaelh updated this revision to Diff 26079.
michaelh added a comment.
Leave early w/o files
Fix comments
Rename setFilesItems to setFileItems
Apply coding style
REPOSITORY
R824 Baloo Widgets
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10105?vs=26068=26079
BRANCH
elvisangelaccio edited the summary of this revision.
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin, #frameworks
elvisangelaccio added inline comments.
INLINE COMMENTS
> filemetadataprovider.cpp:140
> +} else {
> m_data = files.first();
> +insertSingleFileBasicData();
This will crash if `files` is empty, I think?
> filemetadataprovider.cpp:195
> +allDirectories &=
michaelh updated this revision to Diff 26068.
michaelh added a comment.
Correct merging errors
REPOSITORY
R824 Baloo Widgets
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10105?vs=26058=26068
BRANCH
requestfinished (branched from master)
REVISION DETAIL
michaelh planned changes to this revision.
michaelh added a comment.
There are merging errors
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin, #frameworks
michaelh updated this revision to Diff 26058.
michaelh added a comment.
Separate coding-style refactor into extra commit
REPOSITORY
R824 Baloo Widgets
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10105?vs=25960=26058
BRANCH
requestfinished (branched from master)
REVISION
michaelh added a dependency: D10143: baloo-widgets: Apply coding style to
filemetadataprovider .
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin, #frameworks
elvisangelaccio added a comment.
I'd split this into 2 patches: one for the code style changes, and another
one for the actual code refactoring.
This way if the refactoring breaks something, it will be easier to figure out
where the problem is.
REPOSITORY
R824 Baloo Widgets
REVISION
michaelh added a dependent revision: D10113: Emit metaDataRequestFinished once
per request.
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin, #frameworks
ngraham edited the summary of this revision.
REPOSITORY
R824 Baloo Widgets
REVISION DETAIL
https://phabricator.kde.org/D10105
To: michaelh, elvisangelaccio, ngraham, vhanda, smithjd, #dolphin, #frameworks
michaelh created this revision.
michaelh added reviewers: elvisangelaccio, ngraham, vhanda, smithjd, Dolphin,
Frameworks.
michaelh requested review of this revision.
REVISION SUMMARY
Prepare fixing bug 388583
Make signal emission more obvious
Make is easier to distinguish synchronous and
27 matches
Mail list logo
