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

Command palette #656

Merged
merged 23 commits into from
Jan 16, 2023
Merged

Command palette #656

merged 23 commits into from
Jan 16, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Dec 30, 2022

I had some time to kill on the plane.

Try it with cmd-P (mac) or ctrl-P (Windows/Linux)

command-palette

TODO

  • Self-review
  • Decide on a good keyboard shortcut for the command palette
    • Test it on the web

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

@emilk emilk force-pushed the emilk/command-palette branch from 55d608d to d5a376e Compare January 16, 2023 01:49
@emilk emilk marked this pull request as ready for review January 16, 2023 01:49
@teh-cmc teh-cmc self-requested a review January 16, 2023 09:58
Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

The only thing that might be considered blocking is the issue with focus: sometimes the camera still has focus when typing into the command palette and starts moving around.

Comment on lines +171 to +201
pub fn step_time_back(&mut self, times_per_timeline: &TimesPerTimeline) {
let Some(time_values) = times_per_timeline.get(self.timeline()) else { return; };

self.pause();

if let Some(time) = self.time() {
#[allow(clippy::collapsible_else_if)]
let new_time = if let Some(loop_range) = self.active_loop_selection() {
step_back_time_looped(time, time_values, &loop_range)
} else {
step_back_time(time, time_values).into()
};
self.set_time(new_time);
}
}

pub fn step_time_fwd(&mut self, times_per_timeline: &TimesPerTimeline) {
let Some(time_values) = times_per_timeline.get(self.timeline()) else { return; };

self.pause();

if let Some(time) = self.time() {
#[allow(clippy::collapsible_else_if)]
let new_time = if let Some(loop_range) = self.active_loop_selection() {
step_fwd_time_looped(time, time_values, &loop_range)
} else {
step_fwd_time(time, time_values).into()
};
self.set_time(new_time);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these the exact same functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

They have been moved from a file without change, so in that sense they are still the same.

Their bodies differ though, if that is what you mean (though similar).


/// Show the command palette, if it is visible.
#[must_use = "Returns the command that was selected"]
pub fn show(&mut self, egui_ctx: &egui::Context) -> Option<Command> {
Copy link
Member

Choose a reason for hiding this comment

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

didn't we decide on calling of these things something_ui in the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do it for everything that takes a ui: &mut egui::Ui. This is slightly different high-level thing that creates a Ui (in this case creates an area where to put stuff). We haven't decided on a consistent naming for that.

You are right about the other functions though.

Comment on lines +81 to +82
scroll_to_selected_alternative |= ui.input().key_pressed(Key::ArrowUp);
scroll_to_selected_alternative |= ui.input().key_pressed(Key::ArrowDown);
Copy link
Member

Choose a reason for hiding this comment

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

It'd be (very) nice to support ctrl+k/ctrl+p for up and ctrl+j/ctrl+n for down; arrow keys are so painful to reach...

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't you configure your OS to emit arrow-up/down when those keys are pressed?

Comment on lines +267 to +270
let commands = self.pending_commands.drain(..).collect_vec();
for cmd in commands {
self.run_command(cmd, frame, egui_ctx);
}
Copy link
Member

Choose a reason for hiding this comment

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

do you really need the collection here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup - run_command takes &mut self

@emilk
Copy link
Member Author

emilk commented Jan 16, 2023

Part of this: emilk/egui#2598

@emilk emilk merged commit b7a1c6d into main Jan 16, 2023
@emilk emilk deleted the emilk/command-palette branch January 16, 2023 14:55
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