[GH-ISSUE #242] How to change instantiation properties during widget redrawing? #132

Open
opened 2026-03-03 16:22:39 +03:00 by kerem · 5 comments
Owner

Originally created by @flaviostutz on GitHub (Jul 30, 2020).
Original GitHub issue: https://github.com/mum4k/termdash/issues/242

Originally assigned to: @mum4k on GitHub.

I am creating a monitoring dashboard in which I want to change the color of slackline, for example, depending on other things happening on the system (not the value itself, which is not possible too).

For doing this, I made something very questionable, but it worked:

  • During startup, I created the widget instance, placed on the container tree and stored its reference
	h.sparklineDanger, err = sparkline.New(
		sparkline.Color(cell.ColorYellow),
	)
	h.dangerSeries = &ts
  • On dashboard update loop, I used Controller API so that I could control the "redraw()" refreshs along with my logic for changing colors depending on other situations
     func update() {
	sparklineDanger2, err := sparkline.New(
		sparkline.Color(dangerColor()),
	)
        //replace the instance data from previously widget set to the component tree with the new instance
	*h.sparklineDanger = *sparklineDanger2
	for _, dv := range h.dangerSeries.Values {
		h.sparklineDanger.Add([]int{int(dv.Value)})
	}
    }

For the real code, see https://github.com/flaviostutz/perfstat/blob/master/cli/home.go

Is there a better way to do this kind of property modifications or it could be better exposed on termdash API?

Originally created by @flaviostutz on GitHub (Jul 30, 2020). Original GitHub issue: https://github.com/mum4k/termdash/issues/242 Originally assigned to: @mum4k on GitHub. I am creating a monitoring dashboard in which I want to change the color of slackline, for example, depending on other things happening on the system (not the value itself, which is not possible too). For doing this, I made something very questionable, but it worked: * During startup, I created the widget instance, placed on the container tree and stored its reference ```golang h.sparklineDanger, err = sparkline.New( sparkline.Color(cell.ColorYellow), ) h.dangerSeries = &ts ``` * On dashboard update loop, I used Controller API so that I could control the "redraw()" refreshs along with my logic for changing colors depending on other situations ```golang func update() { sparklineDanger2, err := sparkline.New( sparkline.Color(dangerColor()), ) //replace the instance data from previously widget set to the component tree with the new instance *h.sparklineDanger = *sparklineDanger2 for _, dv := range h.dangerSeries.Values { h.sparklineDanger.Add([]int{int(dv.Value)}) } } ``` For the real code, see https://github.com/flaviostutz/perfstat/blob/master/cli/home.go Is there a better way to do this kind of property modifications or it could be better exposed on termdash API?
Author
Owner

@mum4k commented on GitHub (Jul 30, 2020):

I think there may be a simple solution. If you look at the signature of these two functions:

The constructor:
github.com/mum4k/termdash@660ef91c20/widgets/sparkline/sparkline.go (L52-L53)

Method that updates the widget's value:
github.com/mum4k/termdash@660ef91c20/widgets/sparkline/sparkline.go (L145-L159)

You'll notice that they both take optional variadic argument of type Option. You can try passing in the sparkline.Color(...) option on a call to Add() and assuming the code doesn't have some obvious bug, it may just do what you need.

Feel free to let me know if you have any further questions or if this doesn't work.

<!-- gh-comment-id:666084696 --> @mum4k commented on GitHub (Jul 30, 2020): I think there may be a simple solution. If you look at the signature of these two functions: The constructor: https://github.com/mum4k/termdash/blob/660ef91c201729219ef3a8c513a533af057ec81e/widgets/sparkline/sparkline.go#L52-L53 Method that updates the widget's value: https://github.com/mum4k/termdash/blob/660ef91c201729219ef3a8c513a533af057ec81e/widgets/sparkline/sparkline.go#L145-L159 You'll notice that they both take optional variadic argument of type Option. You can try passing in the `sparkline.Color(...)` option on a call to `Add()` and assuming the code doesn't have some obvious bug, it may just do what you need. Feel free to let me know if you have any further questions or if this doesn't work.
Author
Owner

@flaviostutz commented on GitHub (Aug 1, 2020):

Great! As always, you nailed it!

What about Button? What is the "magic" method for it? Looking at the code I couldn't find. What do you thing about creating a Button.Update(opts ... Option) method?

<!-- gh-comment-id:667566136 --> @flaviostutz commented on GitHub (Aug 1, 2020): Great! As always, you nailed it! What about Button? What is the "magic" method for it? Looking at the code I couldn't find. What do you thing about creating a Button.Update(opts ... Option) method?
Author
Owner

@mum4k commented on GitHub (Aug 5, 2020):

True, not all widgets provide this functionality consistently. Adding an Update() method to the button isn't a bad idea.

The other option would be to do something similar to what you did before (create a new button and replace the old one). There actually is an infrastructure supported way of doing that using the Dynamic Layout feature.

Let me know if you are interested in sending a PR that will add the Update method or if you have any further questions about the dynamic layout.

<!-- gh-comment-id:668922307 --> @mum4k commented on GitHub (Aug 5, 2020): True, not all widgets provide this functionality consistently. Adding an `Update()` method to the button isn't a bad idea. The other option would be to do something similar to what you did before (create a new button and replace the old one). There actually is an infrastructure supported way of doing that using the [Dynamic Layout](https://github.com/mum4k/termdash/wiki/Dynamic-layout) feature. Let me know if you are interested in sending a PR that will add the Update method or if you have any further questions about the dynamic layout.
Author
Owner

@flaviostutz commented on GitHub (Aug 7, 2020):

Great... I tried to create an "Update()" method but the results weren't good, so I would have to to deeper. If you have some time, please take a look at https://github.com/flaviostutz/termdash/blob/%23242-add-opts/widgets/button/button.go on method "ApplyOpts(..)" and pls tell me what's is wrong. In my tests the layout got messed up.

My app has a severe memory leak and I think it is due to the updating the pointer variable contents somehow (have to go deeper on this). I didn't know the dynamic layout feature. I will surely try it later.

Thanks again for the great support you provide!

<!-- gh-comment-id:670552807 --> @flaviostutz commented on GitHub (Aug 7, 2020): Great... I tried to create an "Update()" method but the results weren't good, so I would have to to deeper. If you have some time, please take a look at https://github.com/flaviostutz/termdash/blob/%23242-add-opts/widgets/button/button.go on method "ApplyOpts(..)" and pls tell me what's is wrong. In my tests the layout got messed up. My app has a severe memory leak and I think it is due to the updating the pointer variable contents somehow (have to go deeper on this). I didn't know the dynamic layout feature. I will surely try it later. Thanks again for the great support you provide!
Author
Owner

@mum4k commented on GitHub (Aug 7, 2020):

Looking at the ApplyOpts() function, the content seems ok, the only problem I see is that it introduces a race condition, because it modifies the state of the button without holding the corresponding lock (mutex). All that is needed is adding the lock/unlock statement at its beginning, i.e.:

func (b *Button) ApplyOpts(text string, opts ...Option) error {
	b.mu.Lock()  // Acquires mutex
	defer b.mu.Unlock() // Releases mutex

	b.text = text
	opt := newOptions(b.text)
	for _, o := range opts {
		o.set(opt)
	}
	if err := opt.validate(); err != nil {
		return err
	}
	b.opts = opt
	return nil
}

Without knowing more details about why the results weren't good, this may be the best guess I can provide. Please let me know if this helped. If not, try to provide more details about the behavior you are observing.

Updating the pointer shouldn't cause a long-term memory leak. Sure there will be larger memory usage for a while until Go's garbage collector decides to release memory used by the old instances, but that should even out. If you see continuous increase of memory usage, I would recommend trying to profile the memory usage. You can upload the resulting profile diagram here and we can look at it together.

Feel free to let me know if you would like to hear more details about race condition, mutexes or profiling, happy to answer questions if I do know the answers.

<!-- gh-comment-id:670707721 --> @mum4k commented on GitHub (Aug 7, 2020): Looking at the `ApplyOpts()` function, the content seems ok, the only problem I see is that it introduces a race condition, because it modifies the state of the button without holding the corresponding lock (mutex). All that is needed is adding the lock/unlock statement at its beginning, i.e.: ```go func (b *Button) ApplyOpts(text string, opts ...Option) error { b.mu.Lock() // Acquires mutex defer b.mu.Unlock() // Releases mutex b.text = text opt := newOptions(b.text) for _, o := range opts { o.set(opt) } if err := opt.validate(); err != nil { return err } b.opts = opt return nil } ``` Without knowing more details about why the results weren't good, this may be the best guess I can provide. Please let me know if this helped. If not, try to provide more details about the behavior you are observing. Updating the pointer shouldn't cause a long-term memory leak. Sure there will be larger memory usage for a while until Go's garbage collector decides to release memory used by the old instances, but that should even out. If you see continuous increase of memory usage, I would recommend trying to [profile the memory usage](https://blog.golang.org/pprof). You can upload the resulting profile diagram here and we can look at it together. Feel free to let me know if you would like to hear more details about race condition, mutexes or profiling, happy to answer questions if I do know the answers.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/termdash#132
No description provided.