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.

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

Avoid duplicate quick access entry (fixes #152) #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bananeweizen
Copy link
Contributor

When you have used quick access previously, the dialog adds matching
entries from "previous" at the top. Afterwards the same entries may
appear from their "normal" provider (e.q. views, commands, ...).

Since the logic of adding entries seems quite complex, this change fixes
the duplicate entry by detecting the exact situation (same single entry
in first and second provider list), and then removes the duplicate entry
list.

@mickaelistria
Copy link
Contributor

In my use-case reported in #152, I see that both entries are "Previous" so it seems like the issue is something we can better fix earlier without implementing a dedicated mitigation later. Maybe the Previous provider hosts a bug?


// if one category provides the same single entry like "previous", we can avoid
// showing the duplicate second list
if (res.size() >= 2 && res.get(0).size() == 1 && res.get(1).size() == 1
Copy link
Contributor

@Wittmaxi Wittmaxi Sep 6, 2023

Choose a reason for hiding this comment

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

I there a good way to increase clarity by extracting these boolean-statements into either a method or separate variables? For example, a

boolean isDuplicateCategory = ...

would help make the code speak for itself

Copy link
Member

Choose a reason for hiding this comment

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

just wondering , what is the next action on this PR @Wittmaxi is the above suggestion a blocker ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@elsazac no, I was just wondering if the code-quality could be improved. I think this is fine

Copy link
Member

Choose a reason for hiding this comment

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

This can have some reviews.

When you have used quick access previously, the dialog adds matching
entries from "previous" at the top. Afterwards the same entries may
appear from their "normal" provider (e.q. views, commands, ...).

Since the logic of adding entries seems quite complex, this change fixes
the duplicate entry by detecting the exact situation (same single entry
in first and second provider list), and then removes the duplicate entry
list.
Copy link
Contributor

Test Results

   918 files  +    1     918 suites  +1   1h 13m 11s ⏱️ + 36m 56s
 7 507 tests ±    0   7 355 ✅ ±    0  151 💤 ±  0  1 ❌ ±0 
23 670 runs  +1 573  23 164 ✅ +1 454  505 💤 +119  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 344f1d1. ± Comparison against base commit 0de600a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants