Closed Syntaf closed 8 years ago
It's great that there's two of us now playing in the UI code! I do have reservations about the elegance of the API, but because I don't have anything better to propose, I'll have to stick to what I wrote earlier that we shouldn't stand in the way of rapid development at this stage. I hope that with time, we'll figure out a more elegant API and that it will happen before we stabilize it :)
After a quick examination however, I think there might be a couple of bugs lying around in this PR but I don't have the time to review right now. Would you mind giving me a few days to do a proper review?
Of course! this was a rough implementation of the verticallayout struct that I wanted to get pushed into the master branch so I could continue development of a personal project that uses this repo, I'll be looking over it and hopefully refining it when I run into some more free time this week
I think that VerticalLayout
's height being elems.len() -1
is wrong. If a layout has 3 elements, its height is 3. This is, I think, why you had to mangle your margin so much in draw_buttons_subset()
. The commit at https://github.com/hsoft/rustty/commit/72773a9edf5087170369a78ad1f4ad4f2d8d0901 fixes the problem and we can now pass a much more intuitive margin to align()
, that is, 1.
@hsoft ahhh good catch. Do you think buttonlayout.rs
should remain a separate file or be merged into ui.rs
, @cpjreynolds thoughts on this as well? The two are pretty similar, I'm not sure if we quite need buttonlayout to be a separate example when I think about it
FYI @Syntaf , my commit wasn't automatically included in your PR. You have to pull it in your branch for it to be in the PR.
Gotcha, done
In this last commit, I've removed needless code duplication.
As I wrote earlier, I wish that we had a more elegant API for what we're trying to do here and I've been trying to fiddle around so that the API user directly uses HorizontalLayout
and VerticalLayout
to perform its alignments before calling draw_buttons()
. This would be more elegant and flexible (the current code doesn't allow much flexibility in, for example, margins in ButtonLayout::Vertical
scenarios).
However, I ran into button ownership issues and I guess you ran into similar ones during development, which shaped your API. I don't have time to come up with the elegant API I'd like, so it will have to wait. I hope we'll refine this later.
Otherwise, the code is fine, +1.
@hsoft I'm working on an experimental improve the API on my fork at the moment. My envision of a clean API for the ui module would focus around taking ideas from Tkinter if you've ever used that python library. Obviously being a terminal library, our lives our much simpler in that we only have one window and will allow users to place widgets to that window. My basic rules for building the UI is:
Widget
, widget will describe the basic action a widget can perform on a window such as drawing and placingWidget
and offer additional functionality, e.g. a Button
trait will wrap Widget
but offer methods related to dealing with button actions. Button
can then be used when writing, say StandardButton
or CheckButton
or if the user want's to design their own button eventadd_button
, taking any Button
type and placing them. I'm awful at explaining this type of stuff, but (only in my opinion) I'd like to see an API similar to
let mut maindlg = Dialog::new(60, 10);
maindlg.window_mut().draw_box();
let mut b1 = StdButton::new("Foo", 'f', ButtonResult::Custom(1));
b1.align(&maindlg, HorizontalAlign::Left, VerticalAlign::Middle, 0);
maindlg.add_button(b1);
// ....
Where were abstracting separate concepts from each other and using a has-a relationship when constructing dialogs and frames. Currently our button is implemented UNDER dialog, we need to make stuff like this separate, and instead design an API where we can ADD a button to a dialog
EDIT: current branch work
Pull request is deprecated via https://github.com/cpjreynolds/rustty/pull/23
List of things changed:
VerticalLayout
The new prototype for drawing buttons it:
layout can be of enum type
Things to note:
Picture of new example:
thoughts? I haven't yet written documentation, as I don't know how well received this will be