-
-
Notifications
You must be signed in to change notification settings - Fork 108
feat(channel): disable sorting #672
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
base: main
Are you sure you want to change the base?
Conversation
4436e97 to
d648d40
Compare
television/app.rs
Outdated
| pub struct AppOptions { | ||
| /// Whether the application should use subsring matching instead of fuzzy | ||
| /// matching. | ||
| /// Whether the application should use subsring matching instead of fuzzy matching. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're changing this line anyways, can you please fix the typo subsring
|
This looks great, nice job 👍🏻 Here are my thoughts: I'm wondering if changing the semantics a bit could be less confusing to users. Although this really disables sorting in nucleo, I feel like we might want to hide that from the user and expose flags that reflect the user's intention rather than what's happening behind the scenes. What I mean by the above is that disabling nucleo's internal sorting is almost always something the user will want to do when they actually care about sorting and so passing I would change the enum SortingStrategy {
#[default]
Unsorted, // the current behavior
Natural, // disabling nucleo's sorting -> entries keep their natural order
// (the order they were streamed in to tv)
Frecency,
Template, // this could for example extract a score from the entry using a template
}This would also avoid having to do things like: #[serde(default = "default_true")]
pub prefer_prefix: bool,
#[serde(default = "default_true")]
pub sort_results: bool,
}
fn default_true() -> bool {
true
} |
|
See #671 (comment) for more context. |
da3dfed to
7ff55dc
Compare
|
@alexpasmantier hey, given that config/channel refactor has landed 🎉 and this PR is supposed to precede #671 is this the right time to look at this one? |
d1362c0 to
bae2241
Compare
32dc349 to
d2034a8
Compare
|
Any success here? Looks like it should fix the case for ctrl+r search results sorting. |
|
I tried to implement |
|
I'm planning to work on this in the coming days. The reason for the delay is that I want to do this as a part of a bigger refactoring which might involve a bit more than a one evening session and have been a bit busy the last couple of weeks. |
📺 PR Description
allow disabling of sorting, which makes ctrl+r way more useful
this partially addresses #194
Checklist