Introduce new error category that is not handled by the ignore list #9386
Conversation
| }; | ||
| std::vector<std::pair<QString, SyncFileItem::Status>> otherStatusItems; |
There was a problem hiding this comment.
Please add a comment that describes the data structure (i.e., what the first and second items in the pair mean).
There was a problem hiding this comment.
LIke
// list of OtherStatusItems with the localised name
std::vector<std::pair<QString, SyncFileItem::Status>> otherStatusItems;
otherStatusItems.reserve(OtherStatusItems.size());
for (const auto &item : OtherStatusItems) {
otherStatusItems.emplace_back(SyncFileItem::statusEnumDisplayName(item), item);
}
There was a problem hiding this comment.
I didn't get that the first couple of times I was reading this code. Perhaps you should reverse the order in the pair?
TheOneRing
force-pushed
the
work/changed_during_upload
branch
from
73d40f5 to
12e79d9
Compare
| @@ -64,23 +64,48 @@ void LocalDiscoveryTracker::slotItemCompleted(const SyncFileItemPtr &item) | |||
| // | |||
| // For failures, we want to add the file to the list so the next sync | |||
| // will be able to retry it. | |||
There was a problem hiding this comment.
As this is right out of hell, please read carefully
| if (item->_instruction != CSYNC_INSTRUCTION_NONE && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA) { | ||
| break; | ||
| } | ||
| Q_FALLTHROUGH(); |
There was a problem hiding this comment.
This is clever, and checkers of course know that macro. But not all people are Qt masters, and a comment what that macro does and how it changes the code flow would be great. As this changes the flow significantly.
There was a problem hiding this comment.
If I start do document the Qt functions I'll be getting no where.
There was a problem hiding this comment.
Hm, you miss my point here.
There was a problem hiding this comment.
I don't get it either. The macro just hides a compiler warning that is enabled for a good reason. It's pretty standard, we use it a lot in the code base, and I think the name describes its purpose well enough. Do you want @TheOneRing to document the reason a switch-case is now used rather than a big if?
There was a problem hiding this comment.
I know the macro, I learned what it does and yes, all good. Other people do not. And I think we should make understanding of code as easy as possible for people who are not experienced. And these Qt Macros are not very intuitive, so a little comment what the intention of this block is helps newcomers, just to lower the barrier. That is my only point. If you think that is not needed, ok.
There was a problem hiding this comment.
It won't help with the readability but on master we can directly use:
https://en.cppreference.com/w/cpp/language/attributes/fallthrough
src/libsync/propagateupload.cpp
Outdated
| propagator()->_anotherSyncNeeded = true; | ||
| done(SyncFileItem::SoftError, tr("Local file changed during sync.")); | ||
| done(SyncFileItem::Message, tr("Local file changed during syncing. It will be resumed.")); |
There was a problem hiding this comment.
I am not sure if "during syncing" is an accurate english term here. Maybe something like The local file is still busy. It will be resumed.?
There was a problem hiding this comment.
I merged that error with the one a few lines above they where pretty identical.
In this case I don't think we need to describe it accurate but yes during sync is more correct grammar.
TheOneRing
force-pushed
the
work/changed_during_upload
branch
from
12e79d9 to
2986bd4
Compare
|
Kudos, SonarCloud Quality Gate passed! |
| propagator()->_anotherSyncNeeded = true; | ||
| done(SyncFileItem::SoftError, tr("Local file changed during sync.")); | ||
| done(SyncFileItem::Message, tr("Local file changed during sync. It will be resumed.")); |
There was a problem hiding this comment.
resumed->retried?
Or maybe: "The next sync will retry to upload the file."
There was a problem hiding this comment.
Delay until next sync?
| if (item->_instruction != CSYNC_INSTRUCTION_NONE && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA) { | ||
| break; | ||
| } | ||
| Q_FALLTHROUGH(); |
There was a problem hiding this comment.
I don't get it either. The macro just hides a compiler warning that is enabled for a good reason. It's pretty standard, we use it a lot in the code base, and I think the name describes its purpose well enough. Do you want @TheOneRing to document the reason a switch-case is now used rather than a big if?
| }; | ||
| std::vector<std::pair<QString, SyncFileItem::Status>> otherStatusItems; |
There was a problem hiding this comment.
I didn't get that the first couple of times I was reading this code. Perhaps you should reverse the order in the pair?
Fixes: #9382
The filter menu will now be sorted by the localised string.
