natinusala / borealis

Hardware accelerated, controller and TV oriented UI library for PC and Nintendo Switch (libnx)
Apache License 2.0
260 stars 83 forks source link

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

Closed H0neyBadger closed 3 years ago

H0neyBadger commented 3 years ago

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.

https://github.com/natinusala/borealis/blob/205e97ab45922fa7f5c9fa6a85d5d686cd50b669/library/include/borealis/absolute_layout.hpp#L37-L39

natinusala commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Hello, I create a branch for experimentation.

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

https://github.com/H0neyBadger/borealis/commit/d028cda16a7b3f2f2978a0f0e4c4d73031d08d96#diff-c688e1861d26b3147b14ad47322590850994e68d9dd14948b9e2684631dc8b88L42-R42

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

https://github.com/H0neyBadger/borealis/commit/d028cda16a7b3f2f2978a0f0e4c4d73031d08d96#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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/H0neyBadger/borealis/commit/c1e9181fb95a5e93d93667cd63b2f373f95f0bc6 thank you a lot for your help and guidance

natinusala commented 3 years ago

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 commented 3 years ago

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) :

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

H0neyBadger commented 3 years ago

Hi, yes you can. ;)

On Sun, 7 Mar 2021, 21:33 Nathan S., notifications@github.com wrote:

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/natinusala/borealis/issues/75#issuecomment-792347490, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3ZUM27U2TQGS6OVDQSQULTCPPJNANCNFSM4UWMNGNA .