giu icon indicating copy to clipboard operation
giu copied to clipboard

fix: Remove hard config flag `ConfigFlagEnablePowerSavingMode`

Open merlindorin opened this issue 3 years ago • 3 comments

Fix: Remove hard config flag ConfigFlagEnablePowerSavingMode to fix undesirable behavior in case of window reuse (with some OpenGL code, for example).

What I'm trying to achieve?

I want to reuse the master window to have a context for the OpenGL application without rewriting all the plumber.

Which issue I'm facing?

Animation is stale if the mouse is not moving (but I'm power-efficient :P ).

https://user-images.githubusercontent.com/2074733/147962749-e5835616-e301-48b3-a1cb-ac42f52223df.mov

Root cause

Master window is created with a hardcoded config flag ConfigFlagEnablePowerSavingMode that cannot be overridden.

Fix

Remove the hardcoded flag (😎 I know I'm a boss... #kidding, it's a dumb fix). This fix is a breaking change.

Alternative fix considered

  1. Add variadic configFlag... I'm not against the design but this break the actual design (for example func NewMasterWindow(title string, width, height int, flags MasterWindowFlags, configFlags ...MasterWindowConfigFlag) *MasterWindow)
  2. Add ConfigFlagEnablePowerSavingMode in available MasterWindowFlags, but we will have to determine what is a configFlag what is not.

We can also add some tests and set ConfigFlagEnablePowerSavingMode as a default to prevent a breaking change. Still, from my point of view, this will be strange in long-term support (the documentation must explicitly describe the default configuration).

Example for case 2:

if len(filterConfigFlag(flags)) == 0 {
  io.SetConfigFlags(imgui.ConfigFlagEnablePowerSavingMode)
}

Feedback and guidance appreciated.

merlindorin avatar Jan 03 '22 17:01 merlindorin

If you need to make animation works, just call giu.Update in certain rate like 30 times per second. Removing PowerSavingMode by default will increase the learning curve for new user, I don't think it's a good choice.

AllenDang avatar Jan 04 '22 03:01 AllenDang

hi thx for the feedback,

I didn't notice giu.Update(). Feel strange to use 2 loops (maybe I can reuse the main loop for forcing re-rendering... have to check).

On the other hand, I don't see the point about the learning curve. It's just a hard config flag set that is not mandatory (only for limiting resource usage). Maybe I'm misleading myself but limiting config flag usage and putting default that cannot be changed does not help to learn... at least it helps to learn that you are not going to use ConfigFlagNavEnableKeyboard or ConfigFlagNavEnableGamepad :P.

Nevermind, I'm just exploring your library and I can update my PR in the way you prefer (add variadic, etc.) if you think it's interesting.

If not, don't hesitate to close it. Your project, your rules, your philosophy... and of course your expertise ;)

merlindorin avatar Jan 04 '22 13:01 merlindorin

@merlindorin I think your idea is cool, make PowerSavingMode as default config, but provide a way to override it. Sure thing, please go ahead. And the proper way to use giu.Update is, start a goroutine and invoke giu.Update inside it to make UI refresh at certain rate.

AllenDang avatar Jan 04 '22 13:01 AllenDang

Closing due to inactivity

gucio321 avatar Aug 23 '23 20:08 gucio321

Thx, sry for the lack of activity.

merlindorin avatar Aug 25 '23 00:08 merlindorin