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

TabWidget #271

Merged
merged 5 commits into from Jul 24, 2020
Merged

TabWidget #271

merged 5 commits into from Jul 24, 2020

Conversation

uniformbuffer
Copy link
Contributor

@uniformbuffer uniformbuffer commented Jul 23, 2020

Hi,
for an application that i'm making i needed a tab widget to manage the user interface, so i have realized it. I think that now it is in a good shape from a functionality point of view, visually it is ugly but at least work correctly. Maybe with some css magic could be better.
Since i saw that there is a tab widget request on the project page, i thought: "why not share?".
Following an example code that i have used during testing:

use orbtk::prelude::*;

fn main() {
      Application::new()
        .window(|ctx| {
            Window::new()
                .title("OrbTk - minimal example")
                .position((100.0, 100.0))
                .size(600.0, 500.0)
                .resizeable(true)
                .child(
                    TabWidget::new()
                    .tab("Tab header 1",TextBlock::new().text("Tab content 1").build(ctx))
                    .tab("Tab header 2",TextBlock::new().text("Tab content 2").build(ctx))
                    .tab("Tab header 3",TextBlock::new().text("Tab content 3").build(ctx))
                    .build(ctx)
                )
                .build(ctx)
        })
        .run();
}

Currently the TabWidget does not share values about tabs because setting them from the outside could lead into inconsistencies because i cannot get some change callbacks that hint the widget to change state. So for now the widget state is managed internally by the TabWidgetState structure.
The widget will not draw everything during the update call, but only what it is changed, so this make it very lightweight.
There are 2 values shared by TabWidget:

  • close_button(bool): should set the visibility of the close button on the right of the tabs, currently not implemented
  • spacing(f64): set the spacing between tabs

I'm writing some other widgets that are less fundamental and more specialized. If you want i can also share those (maybe putting in a separated folder inside widget folder called "extra" or something similar?)
Some of those widget already completed:

  • DiscreteBar: Similar to ProgressBar, but instead of working on 0%-100% work on discrete value that can be setted as "min" and "max", then the current value can be setted with "val". The widget will display the value using Container widgets that look like bars.
  • Fraction: It is a simple TextBlock that have "val" and "max" property and they are used to display a fraction like "3/10". Since they are managed using properties, setting a shared property will make the widget automatically adjust it's values (i have used it in combination with the DiscreteBar)

Hope it could be useful,
Have a good day

@FloVanGH
Copy link
Contributor

Hi,

it looks awesome so far. I can review it deeply tomorrow. If it is ok for you I can fix the styling after the PR is merged 😉.

You are very welcome to create PRs for the other widgets. A sub module extra would be fine, similar o the behaviors sub module.

Thank very much
Have a nice day too

@uniformbuffer
Copy link
Contributor Author

If it is ok for you I can fix the styling after the PR is merged

Sure, i'm still learning how to use the css theming system properly. Also, looking to how you style the widget can guide me to what is the looking design that Orbtk want to achieve, so in future i could also help with design.
I tried to be more clear as possible on comments, but documenting is definitely not my strong point, so if there is something unclear i can improve it. Also, if there are some functionality that you believe could be added or improved, obviously you can do whatever change you want.

You are very welcome to create PRs for the other widgets. A sub module extra would be fine, similar o the behaviors sub module.

Ok, i will improve the documentation of the widgets and i will send tomorrow.

@FloVanGH
Copy link
Contributor

Sure, i'm still learning how to use the css theming system properly. Also, looking to how you style the widget can guide me to what is the looking design that Orbtk want to achieve, so in future i could also help with design.

In the meanwhile we switched from css to a custom RON based solution. You find a short example on the Readme. I have an initial design created by two of my colleagues. Unfortunately it is not complete and created with a propitious tool. When I develop a new widget I use the other widgets as orientation for colors and fonts and use the material design guidelines for sizes.

@FloVanGH FloVanGH self-requested a review July 24, 2020 04:18
@FloVanGH FloVanGH self-assigned this Jul 24, 2020
Copy link
Contributor

@FloVanGH FloVanGH left a comment

Choose a reason for hiding this comment

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

A last few words:

  • Could you add a tabwidget example please? And add the example to the examples README.
  • Add the TabWidget to the Changelog file.
  • Could you please use cargo fmt add the end?

A common appearance of the code is a important point for me. Therefore some of the comments.

The tabheader must be extended that it possible to scroll through the tab header items. There are different concepts how it could be done e.g. with left, right and overflow button or just a simple scroll bar. But it isn't needed now. It could be done in a later update (new PR).

Thank you. It's a really great work and it is also an important widget for OrbTk!

const BODY_CONTAINER: &str = "body_container";

#[derive(Default,AsAny)]
pub struct TabHeaderState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add documentation please.

@@ -0,0 +1,382 @@
use crate::prelude::*;

const HEADER_CONTAINER: &str = "header_container";
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this please.

// --- KEYS --

const HEADER_CONTAINER: &str = "header_container";
const BODY_CONTAINER: &str = "body_container";

// --- KEYS --



widget!(
/// The `TabHeader` widget is used internally to managege tabs headers. Not meant for other uses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: managege => managed

}
}

pub enum Action
Copy link
Contributor

Choose a reason for hiding this comment

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

If it need to be public add documentation please otherwise remove pub.

}

#[derive(Default, AsAny)]
pub struct TabWidgetState {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it need to be public add documentation please otherwise remove pub.


fn select_by_index_internal(&mut self,ctx: &mut Context,mut index: usize)
{
if self.tabs.len() == 0 {return;}//No tabs could be selected if there are no one
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change it to

//No tabs could be selected if there are no one
 if self.tabs.len() == 0 {return;}

please?

}

widget!(
/// The `TabWidget` widget can store and control multiple tabs with arbitrary content. Only the selected tab will show it's content.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a documentation example please?

/// The `TabWidget` widget can store and control multiple tabs with arbitrary content. Only the selected tab will show it's content.
///
/// This example creates a TabWidget:
/// ```rust
///   TabWidget::new()
///       .tab("Tab header 1",TextBlock::new().text("Tab content 1").build(ctx))
///       .tab("Tab header 2",TextBlock::new().text("Tab content 2").build(ctx))
///       .tab("Tab header 3",TextBlock::new().text("Tab content 3").build(ctx))
///       .build(ctx)
/// ```

)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add

// --- Helpers --

please.

}

impl Template for TabWidget {
fn template(self, id: Entity, ctx: &mut BuildContext) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now it is not possible that a child expands over the whole height of the body container. I would suggest to change the template as following:

    self.name("TabWidget").close_button(true).child(
            Grid::new()
                .rows(Rows::new().add(32).add("*"))
                .child(
                    Stack::new()
                        .id(HEADER_CONTAINER)
                        .orientation("horizontal")
                        .spacing(id)
                        .build(ctx),
                )
                .child(
                    Grid::new()
                        .attach(Grid::row(1))
                        .id(BODY_CONTAINER)
                        .build(ctx),
                )
                .build(ctx),
        )
    }

.border_radius(4.0)
.border_width(0.0)
.border_brush("transparent")
.padding((16.0, 0.0, 16.0, 0.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you do me a favor and remove all .0s? e.g. .padding((16, 0, 16, 0))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was sure that they all require floats, sorry

Copy link
Contributor

Choose a reason for hiding this comment

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

not any more. No problem

@uniformbuffer
Copy link
Contributor Author

uniformbuffer commented Jul 24, 2020

All the problems that you have reported should have been fixed. I also took the opportunity to enable the possibility to toggle the visibility of the close button. I have prepared a fast example that allow to check this feature:

use orbtk::prelude::*;
use orbtk::behaviors::MouseBehavior;
fn main() {
      Application::new()
        .window(|ctx| {
            let tab_widget = TabWidget::new()
            .close_button(true)
            .tab("Tab header 1",TextBlock::new().text("Tab content 1").build(ctx))
            .tab("Tab header 2",TextBlock::new().text("Tab content 2").build(ctx))
            .tab("Tab header 3",TextBlock::new().text("Tab content 3").build(ctx))
            .build(ctx);

            Window::new()
                .title("OrbTk - minimal example")
                .position((100.0, 100.0))
                .size(600.0, 500.0)
                .resizeable(true)
                .child(
                    MouseBehavior::new()
                    .on_scroll(move|states,_|
                    {
                        let current = states.get::<TabWidgetState>(tab_widget).get_close_button_visibility();
                        states.get_mut::<TabWidgetState>(tab_widget).set_close_button_visibility(!current);
                        true
                    })
                    .child(tab_widget)
                    .build(ctx)
                )
                .build(ctx)
        })
        .run();
}

In this example, if you scroll the mouse wheel, the close button visibility will be inverted.
Lastly, to pass the close button visibility property to the TabHeader from the TabWidget, i have needed to add &self to create_tab_header function, so i moved it inside the TabWidget functions (so i have not added the // --- Helpers -- comment).

If there are something else to be fixed (or maybe something that i have missed), please tell me, so i will quickly fix it.
Have a nice day

Edit:
Sorry, i missed these:

  • Could you add a tabwidget example please? And add the example to the examples README.

  • Add the TabWidget to the Changelog file.

  • Could you please use cargo fmt add the end?

Just a moment and i will fix them.

Added a TabWidget example.
Executed `cargo fmt`
@uniformbuffer
Copy link
Contributor Author

uniformbuffer commented Jul 24, 2020

Done.
As example i have added the first one because it is more straight forward.
After running cargo fmt i have noticed that other files have been modified on the commit. It is normal?

@FloVanGH
Copy link
Contributor

Yes it’s normal if other files where not formatted with cargo fmt. Thank you I will review your changes tonight 🙂.

@FloVanGH
Copy link
Contributor

Now the TabHeaders are not displayed:

image

@uniformbuffer
Copy link
Contributor Author

I also got this problem but i have fixed it before pushing the commit. Let me check...

@uniformbuffer
Copy link
Contributor Author

uniformbuffer commented Jul 24, 2020

immagine
For me it's working, i have pushed all the changes, so i should have the same code you are using. It is surely caused by the close button visibility that, for some reason, apply also to the tab header. If you want, i can temporary disable this feature while looking for the root of the problem.

Edit:
Nevermind, i found the problem, pushing the fix

@FloVanGH
Copy link
Contributor

  TabHeader::new()
            .visibility(if self.close_button_visibility {
                Visibility::Visible
            } else {
                Visibility::Collapsed
            })

This could it be?

@FloVanGH
Copy link
Contributor

Yes it is. If I remove this code part the tab headers are displayed.

@uniformbuffer
Copy link
Contributor Author

Yes it is. If I remove this code part the tab headers are displayed.

Yes, that is the problem, i set the wrong property, it should be:

  TabHeader::new()
            .close_button(if self.close_button_visibility {
                Visibility::Visible
            } else {
                Visibility::Collapsed
            })

So this set the visibility on button only, not the whole header.
I'm testing the fix and i will commit when finished.

@uniformbuffer
Copy link
Contributor Author

uniformbuffer commented Jul 24, 2020

Done, sorry for the inconvenience.
On my test code was working because there was 1 more line that set the visibility property from the outside of the widget.
I have noticed it because mine window header name was different from yours. Sorry again

@FloVanGH
Copy link
Contributor

Hi no problem. Thank you for your work.

@FloVanGH FloVanGH merged commit e8dce0a into redox-os:develop Jul 24, 2020
@FloVanGH
Copy link
Contributor

Merged ;-). WIll make it a little bit more styleable and adjust the style a little bit ok?

@uniformbuffer
Copy link
Contributor Author

Merged ;-). WIll make it a little bit more styleable and adjust the style a little bit ok?

Of course, it really needs it ^^

@FloVanGH
Copy link
Contributor

@uniformbuffer styling is done.

@uniformbuffer
Copy link
Contributor Author

@uniformbuffer styling is done.

I'm seeing it right now. Wow, that does not even look like the same widget. It's a very good styling job 😄

@FloVanGH
Copy link
Contributor

Thank you :-). Good styling very good base ;-)

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.

None yet

2 participants