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

Remove duplicated suggestions in awesome bar #1064

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Conversation

HollowMan6
Copy link
Collaborator

@HollowMan6 HollowMan6 commented Oct 13, 2023

We now only remove duplicated suggestions based on URL, maybe later we can decide to remove those entries that only differ in 'http'/'https', or end with '/' or not

Before:
image

After:
image

We now only remove duplicated suggestions based on url,
maybe later we can decide to remove those entries that
only differ in 'http'/'https', or end with '/' or not

Signed-off-by: Songlin Jiang <[email protected]>
@HollowMan6 HollowMan6 force-pushed the awesomebar-duplicated branch 2 times, most recently from 5929cde to b13efad Compare October 13, 2023 17:21
@HollowMan6 HollowMan6 force-pushed the awesomebar-duplicated branch 2 times, most recently from 28678d3 to 391cdf2 Compare October 18, 2023 18:07
Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

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

I understand the rationale of the change but I am not sure it can be successfully resolved by filtering. For example imagine that you have a bookmark with a trailing slash and also a history entry without the slash. Then the history entry will be shown, but not the bookmark. That for me is a mistake because bookmarks must be always shown IMO.

The other comment I have is that comparing strings is normally slow, so I'd use a hash of the URL to be inserted in the Map instead of the full URL.

Last but not least, I don't think we can freely assume that we can remove insecure schemes (like http). For example I have some personal services at home that can be accessed either by http or https as I don't really care too much since they are internal services. I'd presume that is the same in corporate environments.

@HollowMan6
Copy link
Collaborator Author

I understand the rationale of the change but I am not sure it can be successfully resolved by filtering. For example imagine that you have a bookmark with a trailing slash and also a history entry without the slash. Then the history entry will be shown, but not the bookmark. That for me is a mistake because bookmarks must be always shown IMO.

The other comment I have is that comparing strings is normally slow, so I'd use a hash of the URL to be inserted in the Map instead of the full URL.

Last but not least, I don't think we can freely assume that we can remove insecure schemes (like http). For example I have some personal services at home that can be accessed either by http or https as I don't really care too much since they are internal services. I'd presume that is the same in corporate environments.

Okay, anyway we can revert back to 5929cde to make it less aggressive, and modify that so bookmarks must be always shown.

@HollowMan6 HollowMan6 force-pushed the awesomebar-duplicated branch from 391cdf2 to 5929cde Compare October 27, 2023 18:26
@HollowMan6
Copy link
Collaborator Author

I understand the rationale of the change but I am not sure it can be successfully resolved by filtering. For example imagine that you have a bookmark with a trailing slash and also a history entry without the slash. Then the history entry will be shown, but not the bookmark. That for me is a mistake because bookmarks must be always shown IMO.
The other comment I have is that comparing strings is normally slow, so I'd use a hash of the URL to be inserted in the Map instead of the full URL.
Last but not least, I don't think we can freely assume that we can remove insecure schemes (like http). For example I have some personal services at home that can be accessed either by http or https as I don't really care too much since they are internal services. I'd presume that is the same in corporate environments.

Okay, anyway we can revert back to 5929cde to make it less aggressive, and modify that so bookmarks must be always shown.

Reverted. We use hashSet so it's fast to check if a URL already exists. Also we sort before we remove duplication so that only those duplicated results that appear at the bottom of the list will get removed

@HollowMan6 HollowMan6 requested a review from svillar October 27, 2023 18:32
Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

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

This more conservative version looks more appealing to me. Thanks!

@svillar svillar merged commit 5c98a83 into main Oct 30, 2023
18 checks passed
@svillar svillar deleted the awesomebar-duplicated branch October 30, 2023 09:08
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.

3 participants