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

How to change instantiation properties during widget redrawing? #242

Open
flaviostutz opened this issue Jul 30, 2020 · 5 comments
Open

How to change instantiation properties during widget redrawing? #242

flaviostutz opened this issue Jul 30, 2020 · 5 comments
Assignees
Labels
question Further information is requested

Comments

@flaviostutz
Copy link

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?

@mum4k mum4k self-assigned this Jul 30, 2020
@mum4k mum4k added the question Further information is requested label Jul 30, 2020
@mum4k
Copy link
Owner

mum4k commented Jul 30, 2020

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

The constructor:

// New returns a new SparkLine.
func New(opts ...Option) (*SparkLine, error) {

Method that updates the widget's value:

// Add adds data points to the SparkLine.
// Each data point is represented by one bar on the SparkLine. Zero value data
// points are valid and are represented by an empty space on the SparkLine
// (i.e. a missing bar).
//
// At least one data point must be provided. All data points must be positive
// integers.
//
// The last added data point will be the one displayed all the way on the right
// of the SparkLine. If there are more data points than we can fit bars to the
// width of the SparkLine, only the last n data points that fit will be
// visible.
//
// Provided options override values set when New() was called.
func (sl *SparkLine) Add(data []int, opts ...Option) error {

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.

@flaviostutz
Copy link
Author

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?

@mum4k
Copy link
Owner

mum4k commented 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.

@flaviostutz
Copy link
Author

flaviostutz commented 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!

@mum4k
Copy link
Owner

mum4k commented 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants