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

Fixed issue 17 #803

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

razvaniliescu
Copy link

Added a new parameter for the Dialog structure to enable vertical orientation of the buttons and modified the draw methods accordingly.

@razvaniliescu razvaniliescu changed the title Fixed issue 143 Fixed issue 17 Nov 10, 2024
Copy link
Owner

@gyscos gyscos left a comment

Choose a reason for hiding this comment

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

Thank you for the work!

I think we should be able to keep a shared implementation in most cases, using various helper methods around Orientation. Let me know if you need some help!

cursive-core/src/views/dialog.rs Outdated Show resolved Hide resolved
cursive-core/src/views/dialog.rs Outdated Show resolved Hide resolved
Some(y) => y,
None => return None,
};
match self.vertical_button_orientation {
Copy link
Owner

Choose a reason for hiding this comment

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

I believe a shared implementation is possible, rather than two entirely separate branches.

Copy link
Author

Choose a reason for hiding this comment

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

I remade the draw_buttons implementation using the Vec2::from_major_minor method.

.cropped(inner_size)
.focused(self.focus == DialogFocus::Content),
);
match self.vertical_button_orientation {
Copy link
Owner

Choose a reason for hiding this comment

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

Here too we should be able to share the implementation.

Either directly take buttons_size as a Vec2 in the function, or:

let buttons_size = Vec2::from_major_minor(self.buttons_orientation, buttons_size, 0)`

Then you can do

let taken = buttons_size + self.borders.combined() + self.padding.combined();

And continue with the shared logic.

Copy link
Author

Choose a reason for hiding this comment

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

I managed to turn it into a single implementation with the advice you provided.

Some(height) => height,
None => return,
};
match self.vertical_button_orientation {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, we should be able to abstract over the orientation and have a single implementation.

Copy link
Author

Choose a reason for hiding this comment

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

I remade the draw implementation. It has a single branch now, with some additional steps if the orientation is vertical.

@@ -555,15 +565,26 @@ impl Dialog {
match result {
EventResult::Ignored => {
match event {
Copy link
Owner

Choose a reason for hiding this comment

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

You could match (event, self.button_orientation) {, then match on (Event::Key(Key::Up), Orientation::Horizontal), which could be easier than adding if self.button_orientation == ... every time.

Copy link
Author

Choose a reason for hiding this comment

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

Done

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