ArthurSonzogni / FTXUI

:computer: C++ Functional Terminal User Interface. :heart:
MIT License
6.64k stars 399 forks source link

Window doesn't update on adding a element to Elements #41

Closed VedantParanjape closed 4 years ago

VedantParanjape commented 4 years ago

I am adding data to Elements output from a separate thread, data is added correctly, but the screen isn't updated on adding a new element, i need to move the focus area with arrow keys for the UI to update, how can i make it update everytime a new element is added to Elements output

ArthurSonzogni commented 4 years ago

A new frame is drawn after processing the last Event. Events can come from several threads (keyboard events, terminal event, operating system event, ...). To get this right, the events are processed behind a thread-safe queue.

To refresh, you can use the thread-safe method: InteractiveScreen::PostEvent(...). You can send Event::Custom.

There is an example here: https://github.com/ArthurSonzogni/FTXUI/blob/9f8bd4cb3260099e734f4975981d9d9a0ee985d2/examples/component/homescreen.cpp#L374

  std::thread update([&screen]() {
    for (;;) {
      using namespace std::chrono_literals;
      std::this_thread::sleep_for(0.05s);
      shift++;
      screen.PostEvent(Event::Custom);
    }
  });
ArthurSonzogni commented 4 years ago

Note that modifying the Elements (aka std::vector) from one thread, while the "UI" thread is drawing them might result in crashes.

VedantParanjape commented 4 years ago

Note that modifying the Elements (aka std::vector) from one thread, while the "UI" thread is drawing them might result in crashes.

What is the safe way to do this?

Using the method you told, causes crashes and doesn't work well as I am using a low powered device. I tried passing the screen object to component class, but I am unable to do so. It gives some error, like function not defined of some sorts.

Is it possible to refresh just one single component, rather than complete screen? I looked up the source, you are always doing element.first.render()

My use case: I am reading from a file descriptor every 100ms, if new data is received it is pushed to Elements variable from the thread. So what i wanna do is, only when i push data, the screen or that component redraws on the screen with updated elements.

ArthurSonzogni commented 4 years ago

Note that modifying the Elements (aka std::vector) from one thread, while the "UI" thread is drawing them might result in crashes.

What is the safe way to do this?

You are modifying and reading your data from two thread. You need to use a mutex to ensure you don't do this at the same time. https://en.cppreference.com/w/cpp/thread/lock_guard

If you don't want to use mutex, you could also use the Receiver class below as a thread-safe queue to request updates from one thread, but execute them on the UI thread. https://github.com/ArthurSonzogni/FTXUI/blob/master/include/ftxui/component/receiver.hpp

Using the method you told, causes crashes Could you investigate why?

and doesn't work well as I am using a low powered device. I don't really see why we have this implication.

I tried passing the screen object to component class, but I am unable to do so. It gives some error, like function not defined of some sorts. You will have to investigate and find what happens here. I don't have enough informations.

Is it possible to refresh just one single component, rather than complete screen? I looked up the source, you are always doing element.first.render()

No, this is currently not possible. This is simpler this way. I could try to print only the required difference, but this would require printing many "displace the cursor to position (x,y)". This wouldn't be better.

What OS are you using? (Windows support is quite limited now)

My use case: I am reading from a file descriptor every 100ms, if new data is received it is pushed to Elements variable from the thread. So what i wanna do is, only when i push data, the screen or that component redraws on the screen with updated elements.

Then, use screen.PostEvent(Event::Custom) only when you need a refresh.

VedantParanjape commented 4 years ago

You are modifying and reading your data from two thread. You need to use a mutex to ensure you don't do this at the same time. https://en.cppreference.com/w/cpp/thread/lock_guard

I thought to do this, but how can i use mutex inside the render function, I don't think I'd need one since Elements is a std::vector<> which is thread safe by default. Since only thread is modifying and the render function is only reading the Elements.

No, this is currently not possible. This is simpler this way. I could try to print only the required difference, but this would require printing many "displace the cursor to position (x,y)". This wouldn't be better.

I solved this issue by defining a global variable to record state.

What OS are you using? (Windows support is quite limited now)

Linux Mint 20 :) I avoid windows like the plague.

Then, use screen.PostEvent(Event::Custom) only when you need a refresh. I put a if loop inside the while loop in thread like this,

    std::thread update([&screen]() {
    for (;;) {
      using namespace std::chrono_literals;
      std::this_thread::sleep_for(std::chrono::milliseconds(1));
      if(update_screen)
      {
          update_screen = false;
          screen.PostEvent(Event::Custom);
      }
    }
    });
ArthurSonzogni commented 4 years ago

I thought to do this, but how can i use mutex inside the render function.

You can lock the mutex inside the Component::Render() method you overrode. In the component using the vector.

I don't think I'd need one since Elements is a std::vector<> which is thread safe by default. Since only thread is modifying and the render function is only reading the Elements.

If one thread is iterating on the std::vector, while the other modify the vector, it might be reallocated elsewhere. Then the first thread will read unallocated area.

    std::thread update([&screen]() {
    for (;;) {
      using namespace std::chrono_literals;
      std::this_thread::sleep_for(std::chrono::milliseconds(1));
      if(update_screen)
      {
          update_screen = false;
          screen.PostEvent(Event::Custom);
      }
    }
    });

Yes, this would work. I would have expected something like this instead:

void NewDataReceivedFromFileDescriptor(...) {
   [...]
   {
     std::lock_guard<std::mutex> lock(mutex);
     [...] // Modify the UI data.
   }
   screen.PostEvent(Event::Custom); // Refresh the UI.
}

Element MyComponent::Render() {
  std::lock_guard<std::mutex> lock(mutex); // Access data modified from another thread.

  return vbox([...])
}
VedantParanjape commented 4 years ago

Great, did this, and it worked.

So, I have completed the subproject.

thanks for the prompt response here.

ArthurSonzogni commented 4 years ago

Great to hear that!

I am guessing your project is: simpPRU I will add a link to it into the FTXUI Readme, It would be nice seeing a screenshot of the UI.

VedantParanjape commented 4 years ago

Great to hear that!

I am guessing your project is: simpPRU I will add a link to it into the FTXUI Readme, It would be nice seeing a screenshot of the UI.

Here's the pic: https://imgur.com/5V1FSm4, Please don't post the link, rather download the image and upload to your repo, as imgur links from anonymous users expire after a week or so.

This is my Google Summer of Code Project, here's the link: https://github.com/VedantParanjape/simpPRU. Do star and share my project if you like it ;)

Regards, Vedant

ArthurSonzogni commented 4 years ago

Here's the pic: https://imgur.com/5V1FSm4, Please don't post the link, rather download the image and upload to your repo, as imgur links from anonymous users expire after a week or so.

Thanks! I was very happy including a link to: https://github.com/AnisBdz/CPU because it showed a really nice interface using FTXUI in their README. I was thinking I could include a link to your repository and see a nice TUI too. It's up to you whether you prefer having one or not. I will add a link to your repository no matter what.

This was my Google Summer of Code Project, here's the link: https://github.com/VedantParanjape/simpPRU. Do star and share my project if you like it ;)

Sure! I will! Note: I happen to work at Google (on chrome). I am very happy to see one of my project being used for GSoC!

VedantParanjape commented 4 years ago

Here's the pic: https://imgur.com/5V1FSm4, Please don't post the link, rather download the image and upload to your repo, as imgur links from anonymous users expire after a week or so.

Thanks! I was very happy including a link to: https://github.com/AnisBdz/CPU because it showed a really nice interface using FTXUI in their README. I was thinking I could include a link to your repository and see a nice TUI too. It's up to you whether you prefer having one or not. I will add a link to your repository no matter what.

You could link my repository for now, in a few days, I'll finish writing docs, which will include a a detailed guide with screenshots, will update you then.

This is my Google Summer of Code Project, here's the link: https://github.com/VedantParanjape/simpPRU. Do star and share my project if you like it ;)

Sure! I will! Note: I happen to work at Google (on chrome). I am very happy to see one of my project being used for GSoC!

Wow, that's great. I have applied for Google SWE Internship 2021. Coding round's on 16th, wish me luck :) I would like to ask some questions about the same, if you are free, could I drop you a mail?

VedantParanjape commented 4 years ago

I thought to do this, but how can i use mutex inside the render function.

You can lock the mutex inside the Component::Render() method you overrode. In the component using the vector.

I don't think I'd need one since Elements is a std::vector<> which is thread safe by default. Since only thread is modifying and the render function is only reading the Elements.

If one thread is iterating on the std::vector, while the other modify the vector, it might be reallocated elsewhere. Then the first thread will read unallocated area.

    std::thread update([&screen]() {
    for (;;) {
      using namespace std::chrono_literals;
      std::this_thread::sleep_for(std::chrono::milliseconds(1));
      if(update_screen)
      {
          update_screen = false;
          screen.PostEvent(Event::Custom);
      }
    }
    });

Yes, this would work. I would have expected something like this instead:

void NewDataReceivedFromFileDescriptor(...) {
   [...]
   {
     std::lock_guard<std::mutex> lock(mutex);
     [...] // Modify the UI data.
   }
   screen.PostEvent(Event::Custom); // Refresh the UI.
}

Element MyComponent::Render() {
  std::lock_guard<std::mutex> lock(mutex); // Access data modified from another thread.

  return vbox([...])
}

I gave this a bit of thought, why don't you make it a std::atomic variable, i.e this.

std::atomic<std::vector<std::shared_ptr<Node>>>

Won't it give flexibility to a user? I mean beginners might not even know what a mutex is !

ArthurSonzogni commented 4 years ago

Wow, that's great. I have applied for Google SWE Internship 2021. Coding round's on 16th, wish me luck :) I would like to ask some questions about the same, if you are free, could I drop you a mail?

Yes sure, no problems. Feel free to do so on the email I am using for git.

I gave this a bit of thought, why don't you make it a std::atomic variable, i.e this. std::atomic<std::vector<std::shared_ptr>> Won't it give flexibility to a user? I mean beginners might not even know what a mutex is !

Here is a link about what std::atomic is: https://stackoverflow.com/a/31978762

You can't have atomic operations on an std::vector. For simple type like int,float,double,... std::atomic can be useful because the compiler can use some special CPU instructions.

Reallocating a memory area somewhere else is not something that can be done atomically.

VedantParanjape commented 4 years ago

Wow, that's great. I have applied for Google SWE Internship 2021. Coding round's on 16th, wish me luck :) I would like to ask some questions about the same, if you are free, could I drop you a mail?

Yes sure, no problems. Feel free to do so on the email I am using for git.

Cool !

I gave this a bit of thought, why don't you make it a std::atomic variable, i.e this. std::atomic<std::vector> Won't it give flexibility to a user? I mean beginners might not even know what a mutex is !

Here is a link about what std::atomic is: https://stackoverflow.com/a/31978762

You can't have atomic operations on an std::vector. For simple type like int,float,double,... std::atomic can be useful because the compiler can use some special CPU instructions.

Reallocating a memory area somewhere else is not something that can be done atomically.

Ohh, I see. I didn't know about this, gotta read more about C++

thinkty commented 3 years ago

Note that modifying the Elements (aka std::vector) from one thread, while the "UI" thread is drawing them might result in crashes.

What is the safe way to do this?

You are modifying and reading your data from two thread. You need to use a mutex to ensure you don't do this at the same time. https://en.cppreference.com/w/cpp/thread/lock_guard ...

Upon reading on the documentation about lock_guard, I found that it would be better to use a scoped_lock instead as it offers a replacement for lock_guard that provides the ability to lock multiple mutexes using a deadlock avoidance algorithm. (So, I guess much robust compared to lock_guard?)

However, I just started learning c++ so I'm not sure if scoped_lock would be an overkill just for the mutex lock. Can I get an opinion on what you think? Thank you.

VedantParanjape commented 3 years ago

Note that modifying the Elements (aka std::vector) from one thread, while the "UI" thread is drawing them might result in crashes.

What is the safe way to do this?

You are modifying and reading your data from two thread. You need to use a mutex to ensure you don't do this at the same time. https://en.cppreference.com/w/cpp/thread/lock_guard ...

Upon reading on the documentation about lock_guard, I found that it would be better to use a scoped_lock instead as it offers a replacement for lock_guard that provides the ability to lock multiple mutexes using a deadlock avoidance algorithm. (So, I guess much robust compared to lock_guard?)

However, I just started learning c++ so I'm not sure if scoped_lock would be an overkill just for the mutex lock. Can I get an opinion on what you think? Thank you.

Hi Thinkty, my memory is a bit rusty, but I don't think using scoped_lock will be any better here, as lock guard does the job and I am using only one mutex not multiple.

Regards Vedant

thinkty commented 3 years ago

I see, thank you!

ArthurSonzogni commented 3 years ago

scoped_lock is always better than lock_guard and should be used.

However, when locking only one mutex, its behavior is equivalent here.

thinkty commented 3 years ago

scoped_lock is always better than lock_guard and should be used.

However, when locking only one mutex, its behavior is equivalent here.

Encountering a contrast of opinion, I think the behavior is equivalent indeed. Also, I think the overhead of using scoped_lock over lock_guard might be quite subtle to have an impact on performance (but I am not sure)

https://stackoverflow.com/questions/59966836/stdscoped-lock-behaviour-with-a-single-mutex http://eel.is/c++draft/thread.mutex#thread.lock.scoped-2

Initializes pm with tie(m...). Then if sizeof...(MutexTypes) is 0, no effects. Otherwise if sizeof...(MutexTypes) is 1, then m.lock(). Otherwise, lock(m...).

VedantParanjape commented 3 years ago

@thinkty if it works don't touch it 😂