Skip to content

Conversation

@ErichDonGubler
Copy link
Contributor

Satisfies #1991.

@ErichDonGubler
Copy link
Contributor Author

I think this is basically ready for review, disclaiming that I'm not familiar with the vast majority of the Sidebery codebase. I haven't brought it out of draft yet, though, because the premise of this feature hasn't been validated by maintainership (see #1991).

@mbnuqw
Copy link
Owner

mbnuqw commented Nov 15, 2025

Thanks, nice feature! Things that I'd change:

  • Create service omnibox.ts and move that code to it. Export one function setupListeners and call it in background.ts: omnibox.setupListeners().
  • Use different keyword, something easily accessible from other, non-latin keyboard layouts. e.g. = (it seems that this symbol is available in all supported locales (de, fr, hu, pl, ru, zh, ja) without switching to en layout, although I could be wrong).
  • Translate default suggestion

You can make it mergable and I'll complete it myself (prob also add switching between panels).

@ErichDonGubler ErichDonGubler force-pushed the omnibox branch 2 times, most recently from 0e1860c to 678384a Compare November 21, 2025 16:16
@ErichDonGubler
Copy link
Contributor Author

I've applied each of your suggestions. There are still several TODO comments that I'll call out as review discussions, but I think this can otherwise be reviewed normally! 😀

@ErichDonGubler ErichDonGubler changed the title Add sb omnibox handler, use it for reopening tabs in a different container Add = omnibox handler, use it for reopening tabs in a different container Nov 21, 2025
})

function matchContainers(input: string): Container[] {
// TODO: order by score of some sort?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: Any suggestions on what to do here?

@mbnuqw
Copy link
Owner

mbnuqw commented Nov 23, 2025

Thanks

order by score of some sort?

I though about sorting by the time of the last usage, but this will require adding logic for saving/loading info of the last N used commands. Too much for the initial implementation, so I think we can leave this as it is, a couple more of chars in the query will be enough for narrowing down the list of suggestions.

At the very least, put an exact match first.

Not sure we need this right now, with only containers as the target. After all, containers usually have a different names? Later, when there will be more commands this can be added with the above mentioned sorting.

Validate we want Sidebery records, not Firefox records

Not sure I understand this correctly. Sidebery mirrors the Firefox containers augmenting them with new features.

@ErichDonGubler ErichDonGubler force-pushed the omnibox branch 2 times, most recently from 244dc20 to f0ed2e0 Compare November 23, 2025 20:16
@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Nov 24, 2025

@mbnuqw: I've eliminated the last two TODOs.

… a couple more of chars in the query will be enough for narrowing down the list of suggestions.

This currently isn't implemented; did you want this in here before merging?

EDIT: Added a minimum limit of 3 for starting matching with c6a794b.

@mbnuqw
Copy link
Owner

mbnuqw commented Nov 24, 2025

Thank you. Going to merge this.

Added a minimum limit of 3 for starting matching

Sorry, (probably didn't convey my thoughts well) I'll revert this as it will make impossible for Japanese and Chinese users to search for one/two-length names of containers (in hieroglyphs). Such limitations are usually added with the large lists of source data that is not our case.

@mbnuqw mbnuqw merged commit ead689b into mbnuqw:v5 Nov 24, 2025
1 of 2 checks passed
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.

2 participants