Improve FolderStatusModel tests #7855
Conversation
src/gui/folderstatusmodel.cpp
Outdated
| @@ -100,6 +100,7 @@ void FolderStatusModel::setAccountState(const AccountState *accountState) | |||
| } | |||
|
|
|||
| endResetModel(); | |||
| // TODO: maybe we need to analyze the previous state of _dirty before signal emitting? | |||
There was a problem hiding this comment.
would something needed to do here or not ?
I would prefer not to have comments with TODO that will still be there in 10 years (I am pretty sure we have such comments already)
There was a problem hiding this comment.
see my comment below
There was a problem hiding this comment.
what is the added value of this comment ?
we do not really want to clutter the code base with comments that will have to be maintained unless they bring value now and likely in the future
src/gui/folderstatusmodel.cpp
Outdated
| // TODO: think about that maybe better to introduce a separate setter for [_dirty] var with emitting of signal if changed | ||
| // TODO: maybe we need to analyze the previous state of _dirty before signal emitting? | ||
| _dirty = true;; |
There was a problem hiding this comment.
sounds like a nice idea
is there a reason not to do it ?
There was a problem hiding this comment.
yes, it is risky changes on my sight, because I don't know the logic of signal consumers, maybe their logic depends from this signal frequency, but ok, will fixed with introducing of setDirty method
There was a problem hiding this comment.
consumer of this signal is here:
desktop/src/gui/accountsettings.cpp
Line 1568 in c0ada1f
connection declared here:
desktop/src/gui/accountsettings.cpp
Line 247 in c0ada1f
Right approach for emitting of signal is very simple:
void FolderStatusModel::setDirty(bool dirty)
{
if (_dirty != dirty) {
_dirty = dirty;
emit dirtyChanged();
}
}
but as I mentioned I'm not sure that changes in frequency of the emitted signal will not affect the consumer's logic for the worse
There was a problem hiding this comment.
there is maybe something I do not get.
is this change needed to get the automated tests to pass ?
if it is needed by the automated tests then they should specify/document the expected behavior
if not then why changing it ?
There was a problem hiding this comment.
added question
why did you decided not to change it after review but changing it before review ?
| if (!_accountState) { | ||
| if (!_accountState || !index.isValid()) { |
There was a problem hiding this comment.
It would be better to use checkIndex
There was a problem hiding this comment.
maybe, but I didn't anything about this method, but why is it better? I don't see the advantages in comparing with simple QModelIndex::isValid, according to QT6 sources https://codebrowser.dev/qt6/qtbase/src/corelib/itemmodels/qabstractitemmodel.cpp.html#3590 this method is convenient for complex verification of model index with possible logging, but it's not our case, I'm sure that QModelIndex::isValid is enough for fast index validation
There was a problem hiding this comment.
I think @claucambra is referring to the recommended way to test indexes within the model classes from Qt
I am convinced that checkIndex was added for some good reasons
src/gui/folderstatusmodel.cpp
Outdated
| return FetchLabel; | ||
| } else { | ||
| return SubFolder; | ||
| if (index.isValid()) { |
There was a problem hiding this comment.
I think checking if it is valid and doing an early return instead would be cleaner
There was a problem hiding this comment.
disagree, sorry, see my comment above
mgallien
changed the title
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Artiom Khachaturian <[email protected]>
src/gui/folderstatusmodel.cpp
Outdated
| @@ -100,6 +100,7 @@ void FolderStatusModel::setAccountState(const AccountState *accountState) | |||
| } | |||
|
|
|||
| endResetModel(); | |||
| // TODO: maybe we need to analyze the previous state of _dirty before signal emitting? | |||
There was a problem hiding this comment.
what is the added value of this comment ?
we do not really want to clutter the code base with comments that will have to be maintained unless they bring value now and likely in the future
…mKhachaturian/next_cloud_desktop into code_challenge/artiom_khachaturian
…dices verification (when expected that index should be valid)
The task depends on #7842 and should be possible to tackle without deeper knowledge of the desktop client code base. #7842 will give you the code skeleton.
The task would be to remove the QSKIP macro, fix the code to not fail and if time allows improve the coverage and see how far you get there within a timeframe of 4 hours.
Any improvement to that coverage would be a win.