Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow to filter issue table by issue type #9023

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Allow to filter issue table by issue type #9023

merged 1 commit into from
Sep 29, 2021

Conversation

erikjv
Copy link
Collaborator

@erikjv erikjv commented Sep 14, 2021

Fixes: #9000

@erikjv
Copy link
Collaborator Author

erikjv commented Sep 14, 2021

PLEASE TEST BEFORE MERGING :-)

@erikjv erikjv requested a review from TheOneRing September 14, 2021 16:22
@erikjv
Copy link
Collaborator Author

erikjv commented Sep 14, 2021 •
edited
Loading

Oh and please change the target branch, it should be 2.9. I cannot find a way to do that... (I forgot to do that when creating the MR)

@TheOneRing TheOneRing changed the base branch from master to 2.9 September 15, 2021 06:44
fmoc
fmoc suggested changes Sep 15, 2021
View reviewed changes
Copy link
Contributor

@fmoc fmoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I failed to choose the right checkbox...

Please fix the test runs.

@erikjv erikjv force-pushed the work/filter-issue-by-type branch 3 times, most recently from 2debfa7 to 484ebc9 Compare September 16, 2021 11:56
@erikjv erikjv requested a review from fmoc September 17, 2021 08:09
@erikjv erikjv force-pushed the work/filter-issue-by-type branch from 484ebc9 to ce53e6b Compare September 17, 2021 10:07
@TheOneRing
Copy link
Contributor 

09-17 12:41:52:557 [ warning default ]:	QSortFilterProxyModel: index from wrong model passed to mapToSource
09-17 12:41:52:557 [ fatal default ]:	ASSERT: "!"QSortFilterProxyModel: index from wrong model passed to mapToSource"" in file C:\_\17a9f6ae\qtbase-everywhere-src-5.12.10\src\corelib\itemmodels\qsortfilterproxymodel.cpp, line 548

TheOneRing
TheOneRing reviewed Sep 17, 2021
View reviewed changes
src/gui/issueswidget.cpp Outdated
}

private:
QSet<SyncFileItem::Status> _filter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you see an issue in converting the enum to QFlags? It would simplify the filter by a lot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment at the enum states that it fits in 4 bits, so I thought that turning that into flags would break something...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes the 4bit optimisation ......

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I change it or not?

@erikjv erikjv force-pushed the work/filter-issue-by-type branch 3 times, most recently from 3552006 to 07baaae Compare September 23, 2021 18:46
@erikjv erikjv force-pushed the work/filter-issue-by-type branch from 07baaae to 883031a Compare September 24, 2021 14:37
@erikjv erikjv requested a review from TheOneRing September 24, 2021 18:34
@erikjv erikjv force-pushed the work/filter-issue-by-type branch 2 times, most recently from ea3cc84 to f6e8b36 Compare September 27, 2021 13:49
@TheOneRing TheOneRing added this to the 2.9.1 milestone Sep 28, 2021
@TheOneRing
Copy link
Contributor

Looks and works nice
image

Please remove success as we will never have an item with status success in the issue tab

TheOneRing
TheOneRing suggested changes Sep 28, 2021
View reviewed changes
src/gui/issueswidget.cpp Outdated

void resetFilter()
{
setFilter({ false, true, true, true, true, true, true, true, true, true, true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

.fill(true);

?

src/gui/issueswidget.cpp Outdated
_statusSortModel->resetFilter();

for (QAction *action : menu->actions()) {
if (action->objectName() == Models::NoFilterObjectName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not optimal but I can't come up by something better atm, can you add a todo?

also

QObject::findChild

src/gui/protocolwidget.cpp
@@ -77,11 +83,13 @@ ProtocolWidget::~ProtocolWidget()
delete _ui;
}

void ProtocolWidget::showHeaderContextMenu(ExpandingHeaderView *header, QSortFilterProxyModel *model)
void ProtocolWidget::showHeaderContextMenu(ExpandingHeaderView *header)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please re add the filter menu, make sure to not duplicate the code.

src/libsync/syncfileitem.h Outdated
@@ -94,6 +94,7 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem
Excluded
};
Q_ENUM(Status)
static constexpr int StatusCount = Excluded + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have the count in the enum it self. Yes that means we would need to touch the switch statements where the status is used. This would also allow us to drop cases where default: is define and ensure we handle all cases.

@TheOneRing TheOneRing modified the milestones: 2.9.1, 2.9.2 Sep 28, 2021
@erikjv erikjv force-pushed the work/filter-issue-by-type branch from f6e8b36 to 952e401 Compare September 29, 2021 12:57
@CLAassistant
Copy link

CLAassistant commented Sep 29, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@erikjv erikjv force-pushed the work/filter-issue-by-type branch 2 times, most recently from c79d27e to b56fbca Compare September 29, 2021 13:09
@erikjv erikjv force-pushed the work/filter-issue-by-type branch from b56fbca to a79cd3b Compare September 29, 2021 13:13
TheOneRing
TheOneRing approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@TheOneRing TheOneRing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay thx

@sonarqubecloud
Copy link

Please retry analysis of this Pull-Request directly on SonarCloud.

@TheOneRing TheOneRing merged commit 2397475 into 2.9 Sep 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the work/filter-issue-by-type branch September 29, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheOneRing TheOneRing TheOneRing approved these changes

@fmoc fmoc Awaiting requested review from fmoc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.10
Development

Successfully merging this pull request may close these issues.

Allow to filter issue table by issue type
4 participants
@erikjv @TheOneRing @CLAassistant @fmoc