cycfi / elements

Elements C++ GUI library
http://cycfi.github.io/elements/
3k stars 229 forks source link

tab widget #23

Closed Xeverous closed 4 years ago

Xeverous commented 4 years ago

I would like to implement tab (and some other widgets) but I need some guidance regarding library's code - everything is so composable that I have a feeling I will write duplicated code most of time time.

I have read the post and checked how button is implemented (array composite of 2 elements) but I still don't get many things:

djowel commented 4 years ago

On one hand, I was thinking there's a lot I need to answer here and for the sake of expediency, I thought about doing a video chat on this, and your other post on radio-boxes. On the other hand, it might also be good to write down my answer that can become basis for documentation. I'm a bit torn on how to proceed, given that I am also in the middle of something else. Allow me some time to think about how best to approach this, OK? A day or two perhaps...

Xeverous commented 4 years ago

Ok, I'm fine with realtime conversation. From my previous experience from work and other collaborations (mostly university stuff) the most productive way to solve such problems was to actually do all of these things together, that is:

Looking forward to your decision.

djowel commented 4 years ago

Sounds good to me. Actually, I'm thinking the same... Let's do it. Email me at djowel-at-gmail-dot-com and let's schedule a video chat.

Xeverous commented 4 years ago

So the design has been quite well-defined during our latest talk, but I have some implementation-related questions:

For now, lets assume the widget is named notebook as it is the best name for me currently and you haven't commended on this.

Example use would be:

// +------+------+------+------+-----+
// | tab0 | tab1 | tab2 | tab3 | ... |
// +------+------+------+------+-----+
// |                                 |
// |            screen               |
// |         (deck element)          |
// |                                 |
// |                                 |
// +---------------------------------+

auto w = vnotebook(
    htile(tab0, tab1, tab2, ...), // bar of tabs
    deck(screen0, screen1, screen2, ...) // deck element which switches depending on tab
);

My questions:

Unrelated question:

djowel commented 4 years ago

This will be a little bit terse, but I'll see if I can reply with more substance tomorrow.

For now, I suggest working on the tab (very similar to radio_button) and refactoring the similarities. You'll have to do some simple canvas drawings for the tab image (un-selected and selected --typically the selected is lighter than the unselected), using this as the basis:

https://github.com/cycfi/elements/blob/develop/lib/src/element/gallery/radio_button.cpp#L10

where:

  1. state: selected or unselected
  2. hilite: mouse overs over the button (showing some feedback that it is clickable)

Then you can make an example using it, again following the radio_button example in the buttons example.

Let's continue when you have a working tab group example (htile of tabs).

Xeverous commented 4 years ago

radio_button::select has some logic duplication (state is checked twice):

   void basic_radio_button::select(bool state)
   {
      if (state != is_selected())
         value(state);
   }

   bool basic_radio_button::is_selected() const
   {
      return value();
   }

   bool layered_button::value() const
   {
      return _state;
   }

   void layered_button::value(bool new_state)
   {
      if (_state != new_state)
         state(new_state);
   }

It could be just:

   void basic_radio_button::select(bool state)
   {
      value(state);
   }
Xeverous commented 4 years ago

Trying to grasp and move upwards the similarities between check_box and radio_button - I don't like the situation that both widgets create copies of the string and actually store 2x (string + state representation widget) instead of 2x state representation widget + 1x string.

Xeverous commented 4 years ago

I think that radio buttons and checkboxes should be using label factory. Then someone could easily htile them to decide whether they want text on the left or right side of the check/radio.

djowel commented 4 years ago

It could be just:

   void basic_radio_button::select(bool state)
   {
      value(state);

I think you assuming that is_selected() and value() are final (which is probably a good assumption).

djowel commented 4 years ago

Trying to grasp and move upwards the similarities between check_box and radio_button - I don't like the situation that both widgets create copies of the string and actually store 2x (string + state representation widget) instead of 2x state representation widget + 1x string.

Yes, I thought so too. That is also why I started the text_reader thing: https://github.com/cycfi/elements/blob/develop/lib/include/elements/element/text.hpp#L23

Xeverous commented 4 years ago

Possible, that migth simply be an "overassumption" considering there are many virtual calls.

Xeverous commented 4 years ago

So, got any rought plans how a checkbox/radio button would store text? How would you compose them using label and text_reader interface?

djowel commented 4 years ago

So, got any rought plans how a checkbox/radio button would store text? How would you compose them using label and text_reader interface?

The basic idea is that one should be able to make a label that uses a string_view, (-- even an entirely generated string, but that is not relevant here). I'm not sure about the API yet (see below). Let's cross that bridge later :-)

For example, I would like to detect or have the user hint on literal strings. In such cases, you don't even need a std::string. And there are many cases where you just want to pass string_view. The string is already stored somewhere else (e.g. some resources for internationalization e.g. a std::map)... Basically a lifetime management issue.

Xeverous commented 4 years ago

You can make multiple constructors:

djowel commented 4 years ago

So, got any rought plans how a checkbox/radio button would store text? How would you compose them using label and text_reader interface?

The basic idea is that one should be able to make a label that uses a string_view, (-- even an entirely generated string, but that is not relevant here). I'm not sure about the API yet (see below). Let's cross that bridge later :-)

For example, I would like to detect or have the user hint on literal strings. In such cases, you don't even need a std::string. And there are many cases where you just want to pass string_view. The string is already stored somewhere else (e.g. some resources for internationalization e.g. a std::map)... Basically a lifetime management issue.

In other words, the source of the text/string should not matter for things like labels. And we need to give the client ways to hint at the source of the string which determines if it will be stored or not.

djowel commented 4 years ago

You can make multiple constructors:

  • const char(&)[N] - this must be a C-string, guuaranteed infinite lifetime
  • std::string_view - user manages lifetime
  • std::string - widget manages lifetime
  • const char* = delete because it is unknown, require the user to wrap the call into string or string_view.

Exactly! But not on the ctor, instead the factory...

Xeverous commented 4 years ago

To summarize the current state:

What is the current state of text_reader? Are you working on it? I would like to move forward with the tabbar but don't want any conflicts.

djowel commented 4 years ago

What is the current state of text_reader? Are you working on it? I would like to move forward with the tabbar but don't want any conflicts.

Please proceed. I'm working n something else.

Xeverous commented 4 years ago

I'm back and ready for more contributions. I see there have been made some changes by @dmacvicar (CMake, CI, GTK)

I can confirm that both master and develop branches build for me, haven't tried GTK on Windows yet.

https://github.com/cycfi/elements/blob/ccb2f2c4107827cb9f9d98e8fb3daf3944e0567f/CMakeLists.txt#L30-L38

djowel commented 4 years ago
  • Is removing infra still planned? I think I could easily edit all assertions and min/max/clamp calls.

No, actually the filesystem namespace workaround moved to it since we removed the boost::filesystem dependency. But this time, infra is a submodule with header only dependencies and the link problems with other boost libs have been fixed (see my commit remarks).

  • What is the current state of text rendering? I want to push tab widget implementation. Has nothing changed since my last reply?

Text rendering is good. I am busy working on "artist": the "canvas" part of Elements made modular and supporting other backends such as Skia, Quartz2D and Direct2D.

  • Is this actually possible (any permutation of cocoa, GTK, win32 on any of WIN/UNIX/APPLE)? IMO the chache strings should be edited to actually represent what is possible on each system.

Very good point. I'd love to see what you can make out of it. I'll probably need something like it in "Artist" with it's different backends.

Xeverous commented 4 years ago

How about removing duplicate string instances in buttons? Various widgets like radio button and check box are creating multiple layered elements in order to display their on/off states but IMO these layers should only contain the icon, string should be just one. If this is not changed before I implement notebook then it will be another thing to refactor and since notebook is more complex than these I'm afraid it will require more effort to change.

djowel commented 4 years ago

How about removing duplicate string instances in buttons? Various widgets like radio button and check box are creating multiple layered elements in order to display their on/off states but IMO these layers should only contain the icon, string should be just one. If this is not changed before I implement notebook then it will be another thing to refactor and since notebook is more complex than these I'm afraid it will require more effort to change.

Ah right. Yes, thanks for reminding me. Would you open an issue for this? It's low prio for now, but we should have some facilities now to do this. Ideally, you should not even have to hold strings if in many cases they are constant literals. The basic issue is how strings are held and how to detect ownership.

Xeverous commented 4 years ago

Opened #87 and #88. Got any information that could help with working on #87?

djowel commented 4 years ago

Opened #87 and #88. Got any information that could help with working on #87?

Sure! I'll post some comments on the issue. Thanks for doing that BTW!

Xeverous commented 4 years ago

My current "starter" implementation, based on v/htile and composite:

    class notebook_element_base
    {

    };

    class vnotebook_element : public notebook_element_base, public composite_base
    {

    };

    template <std::size_t N, std::size_t M>
    auto vnotebook(
        array_composite<N, htile_element> tabs,
        array_composite<M, deck_element> screens)
    {
        static_assert(N == M);
        using composite = array_composite<2, vnotebook_element>;
        using container = typename composite::container_type;
        composite r{};
        r = container{{ share(std::move(tabs)), share(std::move(screens)) }};
        return r;
    }

    class hnotebook_element : public notebook_element_base, public composite_base
    {

    };

    template <std::size_t N, std::size_t M>
    auto hnotebook(
        array_composite<N, vtile_element> tabs,
        array_composite<M, deck_element> screens)
    {
        static_assert(N == M);
        using composite = array_composite<2, hnotebook_element>;
        using container = typename composite::container_type;
        composite r{};
        r = container{{ share(std::move(tabs)), share(std::move(screens)) }};
        return r;
    }

I have no idea how to progress. I don't get why each vtile_element has a vector of floats and why the actual composite needs to inherit from both container and vtile_element. I thought that a composite would store such vtile_elements, not inherit from such type. I can't get how to implement notebook functions, reading v/htile implementation does not help. The only thing I can state now (in regards to design) that notebook should stretch only the screened deck and allocate minimal room for the bar of tabs.

Xeverous commented 4 years ago

I made a sample tab implementation, see 652e32e87f57a6dc36a5b2f13d10653962dc429a in my fork

Xeverous commented 4 years ago

The rendering is obviously far from being correct but you get the point ... as was expected the implementation is an analogic copy of the radio button (only drawing differs). The question is where the duplicated code should be extracted - selectable?

djowel commented 4 years ago
Screen Shot 2020-05-06 at 8 13 27 PM

There. It is working. I added an basic example for it. Check it out. Let's see how you are able to capture that in a gallery API. Pushed in experimental branch.

djowel commented 4 years ago

Ooops. Compile errors on gcc/msvc. See if you can fix it. Otherwise, I'll do it tomorrow.

djowel commented 4 years ago

OK, gcc and clang builds now. It's down to silly msvc. probably a missing include...

Xeverous commented 4 years ago

Yay! It works. I noticed only some warnings, I can fix them.

Now obviously it would be useful to have a notebook factory that frees the library user from writing a function like the make_tabs in the example. But I also see that the retuned vtile is arbitrarily nested so the widget would need to use something like find_composite, am I right?

Xeverous commented 4 years ago

The click result (r) is not used. Unneeded or something was forgotten?

https://github.com/cycfi/elements/blob/4492395705c074ef08b64f77bc94c9d2332e4668/lib/include/elements/element/button.hpp#L191-L205

djowel commented 4 years ago

I just worked on it as fast as I can. So there should be lots of cleaning up to do. I'll go check tomorrow. I'm done for today. Now your challenge is to make that an easy-to-use gallery API :-)

djowel commented 4 years ago

please pull latest. i just fixed an msvc thing.

Xeverous commented 4 years ago

I fixed the warnings.

I don't get this commit (cf2ae3afa154a7c65e43d6dd7c8c9f587c4258bd) - if there is a need for if constexpr depending on the inheritance then it could potentially be a compiler-intepended issue. But the include replaced with forward declaration (circular dependency)? Is this a separate problem? I don't get what did you fixed there.

djowel commented 4 years ago

I don't get this commit (cf2ae3a) - if there is a need for if constexpr depending on the inheritance then it could potentially be a compiler-intepended issue. But the include replaced with forward declaration (circular dependency)? Is this a separate problem? I don't get what did you fixed there.

No. It looks like a compiler bug. I'll see if there's a better fix tomorrow.

Xeverous commented 4 years ago

Coming back to this issue. To summarize and state what is needed:

The thing I can think of right now is a apply-like functions that takes (deck&, tab&...) instead, assigns all handlers and returns nothing. User would need to explicitly provide all widgets that have some external lifetime.

djowel commented 4 years ago
  • tabs and the deck element can be arbitrarily nested. This makes typical factory function composer apporach impossible.

Never say impossible :-) There is a way. Hint: walk the trees!

djowel commented 4 years ago

Hint: walk the trees!

Oh and by doing that, I probably want a generic set of traversals encapsulated in a group of algorithms, similar to find_composite, find_proxy and find_element. These need to be fleshed out and made generic.

Xeverous commented 4 years ago

I have thought about such solution but it didn't appeal to me:

Xeverous commented 4 years ago

One more thing: if you continue to implement things on this branch, please rebase it before merging. I often need to check something in elements history and having unchronological merge commits does not make it easier.

djowel commented 4 years ago
  • I'm concerned with so pervasive use of RTTI. I don't have a need to make a non-RTTI build, but it concerns me about performance.

Definitely not in this case! This only happens at construction time. And yes of course, as always, when in doubt, don't guess. Measure. I'm pretty sure optimization will be one to deal with, but not at this point. Premature optimization is never a good thing.

Also, there are other techniques beyond RTTI such as static tag dispatch. But I doubt that's the perf issue you are alluding too. Whatever it is, it has to be measured and optimized. I might have an idea, but I do not want to guess.

Yes, I have a few optimization ideas already in mind.

Xeverous commented 4 years ago

Can you implement "walking the tree"? I'm not very knowledgable on find_composite, find_proxy, find_element etc and tabs are my highest-wanted feature since a long time. This feature requires a lot of internal implementation knowledge that only you have. I think you could easily add a function like vnotebook(element& deck, element& tabs) that walks the hierarchy downward and applies handlers on certain elements.

djowel commented 4 years ago

@Xeverous, typically, in the past OSS projects I authored (e.g. Boost Spirit, Fusion, and Phoenix), collaborators go figure out by themselves how things work internally (there were no internal docs). And the internals of Spirit, Fusion, and Phoenix are A LOT more complicated than Elements. To be honest, in the end, it is probably faster for me to implement what you want than explaining the details. But inasmuch as I want to get you up to speed, I have other projects to attend to. But I'll see what I can do to make this a high priority. I appreciate the value of your other contributions and it's probably the least I can do.

Xeverous commented 4 years ago

collaborators go figure out by themselves how things work internally (there were no internal docs). And the internals of Spirit, Fusion, and Phoenix are A LOT more complicated than Elements.

This sounds just bad. Some parts of internal logic are not documented at all and I would be afraid of losing time on questionable implementations or bugs like recent crash in sliders example. This might then cause formal maintainers to need even more time to fix such problems.

I'll see what I can do to make this a high priority. I appreciate the value of your other contributions and it's probably the least I can do.

Yeah, I wanted to point out that I have already contributed a lot (in my free time) (especially recent file dialogs work) so I think expecting some help from you is reasonable. Initially I wanted a modern, clean GUI library and after seeing elements, just only add missing widgets for my needs (radio button, tabs, file dialogs) but I have gone much further than initially expected ... I didn't thought I would be opening 3-digit PRs/issues.

djowel commented 4 years ago

collaborators go figure out by themselves how things work internally (there were no internal docs). And the internals of Spirit, Fusion, and Phoenix are A LOT more complicated than Elements.

This sounds just bad. Some parts of internal logic are not documented at all and I would be afraid of losing time on questionable implementations or bugs like recent crash in sliders example. This might then cause formal maintainers to need even more time to fix such problems.

That is normal in the course of development, esp. before it becomes release ready. That is why some call it the "bleeding edge". Once we get into a stable state, then release 1.0 master will maintain stability, but the develop branch will still experience such things.

It's fine if you are not up to it.

djowel commented 4 years ago

The first cut implementation is on the "notebook" branch. It will be merged to develop once it is stable.

Xeverous commented 4 years ago

I have checked commits on this branch and I'm satisfied with the result.

The only thing worth to mention: the current implementation of notebook factory has hardcoded layout (tabs above pages, left-aligned) but given how the factory looks it should be trivial to add more variations.

djowel commented 4 years ago

The only thing worth to mention: the current implementation of notebook factory has hardcoded layout (tabs above pages, left-aligned) but given how the factory looks it should be trivial to add more variations.

Exactly! As I always say, start simple and add more features later.

Xeverous commented 4 years ago

Ok, my first criticizm of the implementation: https://github.com/cycfi/elements/blob/a3a294aeb3f56361480b998c8eb116e60b40cc58/lib/include/elements/element/gallery/notebook.hpp#L45-L64

I don't like the margin. Try to make something that has a layout similar to a browser. The tabs are not in the top-left corner but some offset from it, caused by the margin in this function. IMO this just looks ugly - a wasted space above the tabs.

My propositions: