Prepare fixing bug 388583
Make signal emission more obvious
Make it easier to distinguish synchronous and asynchronous parts
CCBUG: 388583
Details
Details
- Reviewers
elvisangelaccio ngraham vhanda smithjd - Group Reviewers
Dolphin Frameworks - Commits
- R824:a81ceaeaa963: baloo-widgets: Refactor filemetadataprovider for better readability
Visual inspection
Make test
Diff Detail
Diff Detail
- Repository
- R824 Baloo Widgets
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
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.
| src/filemetadataprovider.cpp | ||
|---|---|---|
| 142–143 | This will crash if files is empty, I think? | |
| 213 | Missing braces here | |
| 315–316 | Please fix this comment, now we don't have 4 code paths inside this function. | |
| src/filemetadataprovider.h | ||
| 125 | Please call it setFileItems(), "fileitem" is kinda used as a single word (e.g. see m_fileItems). | |
Comment Actions
Leave early w/o files
Fix comments
Rename setFilesItems to setFileItems
Apply coding style
| src/filemetadataprovider.cpp | ||
|---|---|---|
| 143 | We can leave early here, I think. 149 insertEditableData() does not make much sense. | |
| src/filemetadataprovider.cpp | ||
|---|---|---|
| 143 | I've been thinking about this... We already have guards against files.isEmpty() in 355 if (!urls.isEmpty()) {and 373 } else if (items.size() == 1) {Unless I got something wrong files cannot be empty at this point. Q_ASSERT(!files.isEmpty) | |
| src/filemetadataprovider.cpp | ||
|---|---|---|
| 143 | Yeah I think you got it right. fetchJob->data() should not be empty as long as we passed a non-empty list of urls to FileFetchJob. Which means an assert about this is a good idea, indeed. | |
Comment Actions
@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.
Comment Actions
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 No Revision Add .arcconfig dataavail Needs Review D10113: baloo-widgets: Emit metaDataRequestFinished once per request unittest No Revision Add '#include <QSignalSpy>' unittest-data No Revision Use camelCase * refactor No Revision Correct typo refactor_space No Revision Remove unrelated white space
arcpatch-D10105 was used only for diagnostic purposes and does not even include changes of
Diff 4 26079 e904962
and following. It is definitely not accepted.
What is going on here?
Comment Actions
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.
Comment Actions
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
Comment Actions
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?
Comment Actions
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.