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

Question about hiding brls::List object form brls::TabFrame #75

Closed
H0neyBadger opened this issue Dec 11, 2020 · 12 comments
Closed

Question about hiding brls::List object form brls::TabFrame #75

H0neyBadger opened this issue Dec 11, 2020 · 12 comments

Comments

@H0neyBadger
Copy link

H0neyBadger commented Dec 11, 2020

Hello,
Is there a way to hide, close or delete a brls::List object form brls::TabFrame ?
like the opposite of the addTab function.

As far as I can see it relies on private vector object. there is a lot of classes involve in this process and I do not want to take bad decision.

private:
std::vector<View*> children;

@natinusala
Copy link
Owner

natinusala commented Dec 14, 2020

Hi, from the code you linked I assume you want to remove from the tab content, in your case an absolute layout.

Do you need to hide it or remove it completely ?

@H0neyBadger
Copy link
Author

Hi, Thank you a lot for your help,
I just followed the path of the addTab function. At the end, all attached views look stored in this vector.
Both solutions (hide/remove) look good for what l need.

I would like to remove/hide the SidebarItem from the brls::TabFrame

(For testing only) I tried to override the addTab function to return its SidebarItem

namespace brls
{

// https://github.com/natinusala/borealis/issues/75
brls::SidebarItem* brls::CustomTabFrame::addTab(std::string label, brls::View* view)
{
    // This custom TabFrame return the SidebarItem pointer
    // for a later use
    brls::SidebarItem* item = this->sidebar->addItem(label, view);
    item->getFocusEvent()->subscribe([this](View* view){
        if (brls::SidebarItem* item = dynamic_cast<SidebarItem*>(view))
        this->switchToView(item->getAssociatedView());
    });

    // Switch to first one as soon as we add it
    if (!this->rightPane)
    {
            Logger::debug("Switching to the first tab");
            this->switchToView(view);
    }
    return item;
}

} // namespace brls

Then, I tried the willDisappear and hide functions (on both the SidebarItem & the Tab View)

    this->willDisappear(true);
    this->hide([]() {});
    // delete this; // crash

But unfortunately objects are just "no visible" and you can still interact with hidden object.

Screenshot from 2020-12-15 06-38-57
Screenshot from 2020-12-15 06-39-04

I know that my tests are not conventional and I do not want to implement that kind of stuff in our project.
So any help is greatly appreciated ;-)

@natinusala
Copy link
Owner

Oh so you want to remove a tab, okay I see.

That's another case of the "it should be in the library already but I never got to do it", the proper way is to add a method to TabFrame to remove tabs.

The thing is, I don't think there is a way to get the "handle" of a tab to remove it later, so you might need to change the addTab method to return an iterator as a handle? Like how events subscriptions work.

addTab returns the handle, and then you do removeTab by giving the handle.

What are the other choices, by index ? Meh

H0neyBadger added a commit to H0neyBadger/borealis that referenced this issue Dec 20, 2020
@H0neyBadger
Copy link
Author

Hello,
I create a branch for experimentation.

and modified the addTab to return the SidebarItem* (for a later removal)

H0neyBadger@d028cda#diff-c688e1861d26b3147b14ad47322590850994e68d9dd14948b9e2684631dc8b88L42-R42

I also edited BoxLayout::removeView to fix all decedent's position.

H0neyBadger@d028cda#diff-de0e0a79c58427c0efed51f2b749d605823dc0d536b4e61886e02754f9aed8b7R154-R165

unfortunately the focus remains on the current view. I have to change dpad direction to fully remove the item.

What do you think, any suggestion ?

(I will have a look to event subscription in order to find a better solution)

@natinusala
Copy link
Owner

Well the iterator solution removes the need for the whole "iterate over all decedent childes to update the position" logic.

As for the focus problem, I think there is no universal solution, where do you think the focus should go?

H0neyBadger added a commit to H0neyBadger/chiaki that referenced this issue Dec 23, 2020
H0neyBadger added a commit to H0neyBadger/borealis that referenced this issue Dec 24, 2020
@H0neyBadger
Copy link
Author

Hi,
To test the iterator solution, I tried to change the "children" type from std::vector to std::list.
(correct me if I m wrong) from the borealis point of view, lists look more appropriate than vector.

unfortunately box_layout relies heavily on Indexes.
and moving from an index system to an iterator system is clearly more difficult than I thought.

the box_layout seems to "add" & "remove" child from the children list regularly.
because of that, I think the first iterator returned from the "addTab" become obsolete pretty quickly.

I'm taking a short break, before discovering that I'm wasting time on the wrong direction.

I keep my experiment commit in a dirty state (with printf and glitches) if you want to have a look at it
H0neyBadger@c1e9181
thank you a lot for your help and guidance

@natinusala
Copy link
Owner

natinusala commented Jan 3, 2021

Hi, sorry for the delay I was on holiday, I am still catching up on things

There is a pretty good example on how to use iterators there: https://github.com/natinusala/borealis/blob/master/library/include/borealis/event.hpp

The advantage of using iterators is that they never become "obsolete", as you say. But that would indeed mean rewrite the entirety of BoxLayout to use iterators instead of indexes.

So, seeing that it's a huge amount of work needed... may I ask, is that for chiaki? What UI elements and borealis features do you use in that homebrew? Could you share a screenshot?

I am asking because I'm currently (on a break of) entirely rewriting the layout system of the library. I am slowly rewriting every component we had to the new system, and it takes time... but it solves the issue you are having, among others. I am saying that to see if you could consider switching to the new version, and maybe help me write what's left (it's a lot)?

So far only AppletFrame, TabFrame, Label and Image are rewritten, but the new code is so, so much better. BoxLayout has been rewritten to allow alignment/gravity, padding, margins, absolute positioning... And there is an XML system to write the interfaces without writing code, exactly like Android.

@H0neyBadger
Copy link
Author

Hi no problems :-) .
from the "dev" point of view, iterators are definitely a good idea.
I carefully read your event example. And that's what I tried to implement it in my draft (But I might misunderstood the point here). my code is working. To get ride completely of the index system, it implies a deep change in many many classes.
from my understanding (in my code draft), and as far as i can see, the current version frequently add/remove childs from the list. when you come back later with your iterator, it no longer pointing to a valid object.

yes, that's for chiaki. (you have a screenshots in this issue, in a previous post)
basically we are using (everything except setup/staged views and i18n) :

  • brls::List (gui base)
  • brls::SelectListItem (for user choice)
  • brls::View (to display opengl video)
  • brls::Dialog
  • crash, notify, ...

yes, I m ok to move or contribute on another version ;-). the current chiaki design needs a refactor anyway.
I m taking time during my holidays to work on personal project. Outside holidays, it will be difficult to work on two different projects.

feel free to send me the link of your repo. I will have a look.

@natinusala
Copy link
Owner

Okay so I see that you are not using "advanced" features. I was planning on working on Button next, but I can do the ListItem family if you need it. Or if you want to help writing the new version, you can do ListItem yourself 😄

The code is here: https://github.com/natinusala/borealis/tree/yoga

It's a big mess (just look at the TODO list in the scrolling box file 🤡 ) but it works, you can compile and run the demo to see for yourself (you might need to remove some WIP header code causing undefined references first 👀).

The files of the demo are here, to give you an idea of what the new code looks like:
https://github.com/natinusala/borealis/blob/yoga/resources/xml/activity/main.xml
https://github.com/natinusala/borealis/blob/yoga/resources/xml/tabs/components.xml
https://github.com/natinusala/borealis/blob/yoga/resources/xml/tabs/settings_list.xml (stub)
https://github.com/natinusala/borealis/blob/yoga/resources/xml/tabs/recycling_list.xml
https://github.com/natinusala/borealis/tree/yoga/demo (C++ part)

There is very few C++ code, and that's on purpose 😏

@H0neyBadger
Copy link
Author

H0neyBadger commented Jan 3, 2021

I just tested it, and it looks pretty good ^^.
I had to apply the following tmp patch to compile from the yoga branch.
I will give a try and check how I can help on it.

diff --git a/library/lib/style.cpp b/library/lib/style.cpp
index 2e37766..6eb8e3a 100644
--- a/library/lib/style.cpp
+++ b/library/lib/style.cpp
@@ -19,7 +19,7 @@
 */
 
 #include <borealis/style.hpp>
-
+#include <stdexcept>
 namespace brls
 {
 
diff --git a/library/lib/theme.cpp b/library/lib/theme.cpp
index ba3b68d..b77a9cb 100644
--- a/library/lib/theme.cpp
+++ b/library/lib/theme.cpp
@@ -18,7 +18,7 @@
 */
 
 #include <borealis/theme.hpp>
-
+#include <stdexcept>
 namespace brls
 {
 
diff --git a/library/lib/view.cpp b/library/lib/view.cpp
index c8f1c3d..9325185 100644
--- a/library/lib/view.cpp
+++ b/library/lib/view.cpp
@@ -1649,6 +1649,7 @@ void View::registerCommonAttributes()
         this->setBackgroundColor(value);
     });
 
+    /*
     this->registerColorXMLAttribute("borderColor", [this](NVGcolor value) {
         this->setBorderColor(value);
     });
@@ -1668,6 +1669,7 @@ void View::registerCommonAttributes()
             { "generic", ShadowType::GENERIC, },
             { "custom", ShadowType::CUSTOM, },
         });
+    */
 
     // Misc
     BRLS_REGISTER_ENUM_XML_ATTRIBUTE(
diff --git a/meson.build b/meson.build
index d4cad3c..3ae659f 100644
--- a/meson.build
+++ b/meson.build
@@ -7,9 +7,7 @@ subdir('library')
 
 demo_files = files(
     'demo/main.cpp',
-
     'demo/main_activity.cpp',
-
     'demo/recycling_list_tab.cpp',
     'demo/captioned_image.cpp',
 )

@natinusala
Copy link
Owner

HI, can I close this issue? I am trying to tidy things up for the master branch which will become unsupported.

@H0neyBadger
Copy link
Author

H0neyBadger commented Mar 7, 2021 via email

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

No branches or pull requests

2 participants