ChrisTitusTech
Create a dynamic hotkey list
>>Wait for the demo to load<<
In this PR I made a bottom bar that displays the correct keyboard shortcuts depending on the context. I also added a credits label in the top left corner.
[!Note]
This pr did not do much refactoring and does not change how this app works on a deeper level.
This should be ready to merge.
Thanks for the feedback, but I just threw all this together as quickly as possible, without refactoring anything, so I would say save it for the evening (Central EU timezone) or for tomorrow, when I will clean everything up. As for wether this should be an impl on Focus, I think that is wrong, I think almost nothing should be in state.rs, and the widgets should be split up into different files, but I want to do the refactoring PR separately, not to make this one too big
And I want to do a huge refactor, but it will brake all other PRs, so I am not sure what is the best aproach there. 👍 For doing a big refactor rn. 👎 If a refactor is not necessary. 👀 If you think a refactor is a good Idea, but I should time it when there are less PRs
Looks good! #285 would go great with this.
Done :heart:! Thanks for the suggestion
I feel like there's a path forward with uniting event handling and shortcut hinting. It would make updating shortcut hints when event handlers/bindings much easier* & more robust
Something like this:
// Non-exhaustive event description
// just an example
pub struct CommandCompleteEvent {
pub exit_code: i32,
pub pid: u32,
// ...
}
pub enum Event {
KeyPress(KeyEvent),
CommandComplete(CommandCompleteEvent)
// ...
}
pub struct EventContext {
propagate: bool
// ...
}
impl EventContext {
pub fn stop_bubbling(&mut self) -> () {
self.propagate = false;
}
pub fn can_continue(&self) -> bool {
self.propagate
}
}
type EventHandler = Fn(Event, EventContext) -> ();
impl AppState {
pub fn register_handler(&mut self, handler: EventHandler) -> u32 {
// ...
}
pub fn remove_handler(&mut self, h_id: u32) -> bool {
// ...
}
}
Keep in mind that I wrote this from my phone since I'm afk, and I can already see some ways that this would need to be fixed up (e.g. some way to specify the type of event to handle rather than just getting all events that could potentially be emitted, etc.), but this is generally speaking what I'm thinking. It's also missing the instrumentation to reflect on the key bindings, but I very clearly see how we could implement that using this kind of event handling architecture.
@cartercanedy I completely understand your desire to abstract here, but In this particular case, I think an abstraction will just not work, and will require a huge rewrite and add way too much complexity. Let my try to explain why I think that way.
The "keyboard events", are pretty easy to route through the application. But currently we handle a key press in a match statement. Therefore to "extract" which keypress corresponds to what function call / handling logic we will need to create a whole keyboard shortcut management system. And doing that will add at least 400 loc to the application. Now that is without even considering the general event management system you are proposing. Such a step is very logical on a scale of Firefox or a KDE, but doing it for a rust CLI app is just overkill, and it's not that hard to change the keyboard shortcuts in 2 places when such changes are necessary.
While I agree that the current state of things is far from perfect, and after this PR + my other PR gets resolved, I will try to refactor each widget into its own file again, and that will even come with some performance improvement, as a super simplified version of "event management" will be easier to implement, and that way we can stop re-drawing the screen every frame, when nothing happens. I also don't like the use of dynamic dispatch in the floating widget implementation, it is not worth it and should be just an enum.
I also don't like the use of dynamic dispatch in the floating widget implementation, it is not worth it and should be just an enum.
Dynamic dispatch heavily simplifies the code, as well as reducing binary size, with a small runtime cost. If you want to remove it, you'll need to draw up an enum mapping out all the types implementing float, and writing functions containing match statements that only call a function under the same name for every single value. Basically, a bunch of boilerplate, with no real benefit.
Just because you may not like the look of the code for the better solution doesn't mean it should be replaced.
I also don't like the use of dynamic dispatch in the floating widget implementation, it is not worth it and should be just an enum.
Dynamic dispatch heavily simplifies the code, as well as reducing binary size, with a small runtime cost. If you want to remove it, you'll need to draw up an enum mapping out all the types implementing float, and writing functions containing match statements that only call a function under the same name for every single value. Basically, a bunch of boilerplate, with no real benefit.
Just because you may not like the look of the code for the better solution doesn't mean it should be replaced.
I agree, perf really isn't that much of an issue here. Double deref hits really don't matter considering the current rendering model.
As well as reducing binary size, with a small runtime cost.
- It doesn't reduce the final binary size AT ALL, the types are still generated and all the functions are still created and present in the final binary, there is no magic, and the vtable only adds a bit of space and not reducing it in any way.
Dynamic dispatch heavily simplifies the code
In this case it doesn't, because we are not implementing a generic floating widget system where each window is a client that talks to a server. It is precisely because it is more annoying to work with I don't like it in this case, nothing against dynamic dispatch in general though.
Why? It is difficult to work with because of how it works, it erases the type of the struct implementing the trait, which creates a need for an interface (the trait itself) to be able to interact with that float. And what happens when the 2 objects we manage want to do something different? We have to add all of that to the interface. Like I did with the get_shortcut_list() function. And instead of writing 1 match statement I have to create 2 functions and an interface to just return me the same thing every time, because I can't know which float is in use rn.
The ideas of abstarct interfaces are cool, but they just make it harder to work on a smaller scale. The runtime cost is not even considered on this type of application. We are re-drawing on all frames, just bk, so talking about a pointer dereference cost would be very hypocritical of me.
If you want to remove it, you'll need to draw up an enum mapping out all the types implementing float, and writing functions containing match statements that only call a function under the same name for every single value. Basically, a bunch of boilerplate, with no real benefit.
It is exactly the opposite, when you have only the necessary methods on both of them, and you can just match on which one it is, it makes the code shorter and easier to understand. Yes it would be impossible to manage 100 floating windows this way, but when the number is 2, I think we are ok :)
Just because you may not like the look of the code for the better solution doesn't mean it should be replaced.
And I agree that code taste is very subjective, but I think you should agree that creating abstractions too early creates way more work then is necessary
- the types are still generated and all the functions are still created and present in the final binary, there is no magic
Moving away from dynamic dispatch would require static dispatch to be used for Float. Static dispatch creates a unique copy of each function for every type it's implemented for.
And what happens when the 2 objects we manage want to do something different?
What else do we need the floats to do? Create sounds? Seriously. This is a pointless hypothetical, the only things a floating window needs to handle are keypresses and drawing itself. You've already added functions which aren't strictly necessary to the trait (is_finished) which have (what should probably be) default implementations.
Creating an enum which will be contained within Focus::FloatingWindow would be no different, you'd have to write this function with a match statement, providing a default implementation for types which don't support the new functionality. There's very little difference.
And instead of writing 1 match statement I have to create 2 functions and an interface to just return me the same thing every time, because I can't know which float is in use rn.
Moving the shortcut list to each specific type implementing float is a far better idea than having a match statement. What if the type implementing float had a struct field which was relevant to which keybinds are available? You would be completely unable to determine that in a match statement. The match statement also would increase in complexity the more floating types you add. This abstraction makes the code far easier to read and write, with essentially zero downside.
and you can just match on which one it is, it makes the code shorter and easier to understand
It's indeed true that it wouldn't be complicated. It would just be unnecessary, when we can use dynamic dispatch instead. Your match statement would look something like this.
pub fn draw(&mut self, frame: &mut Frame, parent_area: Rect) {
match self {
Self::Command(command) => command.draw(frame, parent_area),
Self::Preview(preview) => preview.draw(frame, parent_area),
}
}
Why should we do this when we have dynamic dispatch available, which will handle all of this for us?
but I think you should agree that creating abstractions too early creates way more work then is necessary
Abstractions can indeed create more work. It's just that in this case, with static dispatch and an enum, you'd be implementing nearly exactly the same thing that dynamic dispatch can do for us. If you're concerned with adding more functions to the trait signature, keep in mind that you can add default implementations to a trait. i.e.
trait Float {
// ...
fn is_finished(&self) -> bool { true }
}
This should probably be moved over to a discussion, none of this is relevant to this pr as it currently stands
I am just gona argue the first one, I can't argue on hypotheticals untill I have a POC, so wait for that.
Moving away from dynamic dispatch would require static dispatch to be used for Float. Static dispatch creates a unique copy of each function for every type it's implemented for
Dynamic dispatch uses those copies and puts pointers to them in a struct attached to every pointer. And so the copies are still there
This should probably be moved over to a discussion, none of this is relevant to this pr as it currently stands
Titus liked it the first time :smile:
Titus liked it the first time 😄
Please look at the original pull request which implemented floats with static dispatch: https://github.com/ChrisTitusTech/linutil/pull/137/files. I think you can agree that the code structure there is much worse than what it was replaced with. It was indeed a superior solution than the previously completely separate RunningCommand and Preview implementations.
Dynamic dispatch uses those copies and puts pointers to them in a struct attached to every pointer. And so the copies are still there
Each function implemented within the Float impl block has a copy generated for every type Float is built around (Float<RunningCommand>, Float<FloatingText>, etc), since each function has to point to one specific function for the type (implemented as part of a Trait, or otherwise). Dynamic dispatch creates one of each of those functions, which includes logic to find the correct function to call at runtime instead.
I'm not the best person to explain this, but it goes something like this.
fn _floating_text_draw(...) {
// ...
FloatingText::draw(...)
}
fn _running_command_draw(...) {
// ...
RunningCommand::draw(...)
}
The important part here is the logic before the call to the next draw function. All of that is duplicated. The trait implementations of course have unique functions generated.
I am not entirely against abstractions in this case, but as you said it yourself
Abstractions can indeed create more work. It's just that in this case, with static dispatch and an enum, you'd be implementing nearly exactly the same thing that dynamic dispatch can do for us
But here is the thing, if you can have all the control of static dispatch without writing 10x more code and even having less code, that is 100% worth it in my book.
For example: To know where a widget is, we have to divide the layout a bunch of times in a central-ish location. So it wouldn't make any sense to move rendering of each widget fully into their own file as we would have to redo all the work multiple times to find out where is situated all widgets. So at least if we do decide to separate widgets, they should take in an area: Rect as a parameter to their render / draw function.
But now we have a choice: Do we make a trait all widgets implement, and put them in some kind of an array and manage them that way, or do we (mb with the same trait) manually manage them from the location where their sizes are determined. And in this case it is very clear that unobstructed access to widgets, some of which have different behaviours, focusing behaviours, running other widgets / floats / windows, etc. And putting all that in 1 single trait just makes it harder.
And we have very similar issues with our "Floaters", and the fact that they are managed separately from all other widgets and are very different makes no sense.
Titus liked it the first time 😄
Please look at the original pull request which implemented floats with static dispatch: https://github.com/ChrisTitusTech/linutil/pull/137/files. I think you can agree that the code structure there is much worse than what it was replaced with. It was indeed a superior solution than the previously completely separate RunningCommand and Preview implementations.
- I agree that that particular rewrite was not the best .
- I was not talking about that, I was refering to the fact that Titus liked us arguing :)
Each function implemented within the Float impl block has a copy generated for every type Float is built around (
Float<RunningCommand>,Float<FloatingText>, etc), since each function has to point to one specific function for the type (implemented as part of a Trait, or otherwise). Dynamic dispatch creates one of each of those functions, which includes logic to find the correct function to call at runtime instead.The trait implementations of course have unique functions generated.
Well yes that is all true, but when we don't use dynamic dispatch that doesn't mean we use one more level of generics, a match in a function is also not duplicated, so that's why the argument about saving space is pointless in this scenario. But even if we were talking about multiple levels of generics, the number of bytes you save that way is so small, 1 bash script from Titus will take 10x more space.
@lj3954 Just to make it clear, we are arguing about stuff outside this PR. But do you approve this PR in its current form?
I just wanted to mention it, the binary size is so large not because we did something wrong, but because we are using GIANT crates like vt100, and if the binary size is 1.5 MiB rn, it will probably grow up to 2.5 MiB in the next 5 years. There is no point panicking, Even a 20 MiB binary is nothing, and not a big problem at all
And in this case it is very clear that unobstructed access to widgets, some of which have different behaviours, focusing behaviours, running other widgets / floats / windows, etc. And putting all that in 1 single trait just makes it harder.
The only thing that's going to be changed is that instead of writing a function with a default implementation within the trait (as mentioned above), you're going to be writing a match statement in the implementation of an enum with a default case. There is very little difference between those 2 solutions.
And we have very similar issues with our "Floaters", and the fact that they are managed separately from all other widgets and are very different makes no sense.
It makes perfect sense. The whole point of #137 was that new floating windows could be added with a base structure already there, and that they could then be dropped into the existing code; the logic to use the functions which are part of the trait already being present.
but when we don't use dynamic dispatch that doesn't mean we use one more level of generics, a match in a function is also not duplicated
Your enum signature will look something like this.
enum FloatingWindow {
Preview(Float<FloatingText>),
Command(Float<RunningCommand>),
// ...
}
Once again, each function which is implemented for the Float struct will be duplicated.
But do you approve this PR in its current form?
I've already given approval to this PR.
the binary size is so large not because we did something wrong
Of course. I'm familiar with the binary size of Rust programs, especially when using library crates. A default release build of Hello World is 400KB. Static linking is going to increase the binary size, that's inevitable. It's a fact that static dispatch would increase that size, but it won't be major unless we have hundreds of types implementing this trait.
Of course. I'm familiar with the binary size of Rust programs, especially when using library crates. A default release build of Hello World is 400KB. Static linking is going to increase the binary size, that's inevitable. It's a fact that static dispatch would increase that size, but it won't be major unless we have hundreds of types implementing this trait.
I think it's safe to assume that most people that are using this tool will have gnutils installed, it might be worth gzipping for the network runner.
Of course. I'm familiar with the binary size of Rust programs, especially when using library crates. A default release build of Hello World is 400KB. Static linking is going to increase the binary size, that's inevitable. It's a fact that static dispatch would increase that size, but it won't be major unless we have hundreds of types implementing this trait.
I think it's safe to assume that most people that are using this tool will have gnutils installed, it might be worth gzipping for the network runner.
Even more then that, I just tried upx ing the binary, its a tool that can compress a binary, and I got a reduction from 1.6 -> 400k, But we are talking ones of megabytes, this is not a JS library somebody will have to download 100s of times per day on every website, this is a cli tool, it's ok to be a lil fat :)
@lj3954 When I say duplicated, I don't mean "Needs to be re-written", I mean the generated code will not have duplicates because of the different types involved, as there are no generics. And yes, a match statement will probably consume 5 more bytes per function, but cmon
@lj3954 And I told I wouldn't argue about hypotheticals, and I couldn't keep my word, but I really can't. I have this really cool idea on how to organize all that in a way without dynamic dispatch and without adding much boilerplate and make it extendable, but its getting late, and I probably won't be able to finish it today :) so lets just wait. I think you will like it tbh
I mean the generated code will not have duplicates because of the different types involved, as there are no generics.
Yes, there will be generics. Unless you want to remove the trait entirely, which I don't think is a particularly good idea. struct Float<T: FloatContent> would be the signature of the struct with static dispatch, T is the generic type. And the generated code WILL have duplicates of all functions implemented for Float. That's what static dispatch is. The match statement shows why pretty well. The functions you're calling aren't part of a generic Float implementation, they're part of the implementation for the type you have, Float<RunningCommand> or the like. You're essentially making a separate struct declaration for each generic type.