AllenDang / giu

Cross platform rapid GUI framework for golang based on Dear ImGui.
MIT License
2.11k stars 127 forks source link

add GenAutoID to Context #795

Closed gucio321 closed 1 month ago

gucio321 commented 1 month ago

this allows to disable GenAutoID

AllenDang commented 1 month ago

@gucio321 To stabilize the ID of a widget, you only need to invoke .ID(xxx) on the widget which you don't need the auto-generation.

AllenDang commented 1 month ago

@gucio321 To globally disable the GenAutoID is not necessary.

gucio321 commented 1 month ago

Hi @AllenDang First of all - I'm sorry for doing this without asking you. I should have akded first. In my opinion having a possibility to globally disable auto id generator is a nice alternative to using ID method. E.g.

giu.Button("").ID("my id##id")
// vs
giu.Context.GenAutoID = func(id string) string {return id}
giu.Button("my id##id")

the above when using more widgets seems easier and more logical in use.

And whay would I want to disable audo id? well, it has a bug that, to be honest, I have no idea how to solve atm. If we have a few nested widgets like tree node + tab bar and some layouts are dynamically created, sometimes tabbar/tree node status gets reset.

but there is another use case for the feature from this PR. Users can define their own GenAutoID methods (idk why would they want to do this but, it is possible now just in case).

Also, use of this feature is optional so if someone doesn't need it or doesn't even know abuot it nothing happens.

AllenDang commented 1 month ago

@gucio321 If we have a few nested widgets like tree node + tab bar and some layouts are dynamically created, sometimes tabbar/tree node status gets reset. For these cases, I think we should force user to give an ID for those widgets and do not use GenAutoID for them in the first place, because that will introduce state reset. So problem solved, when a widget needs stable ID to store internal state, force user to provide one. You think?

gucio321 commented 1 month ago

yes and no. In my opinion it will be really confusing. I got a few ideas in the meantime:

  1. we should specify when we need ID (by ID I mean something that will not be changed by GenAutoID). e.g. in such a way I did in cimguigo: define type ID string and when something is an supposed to be an ID it gets ID type
  2. maybe we can extend our GenAutoID mechanism? Lets say the following:
    
    func GenAutoID(id string, widgetType WidgetType) ID {}
    type WidgetType byte
    const(
    // types for all widgets
    )

type Context struct { // ... widgetCounter map[WidgetType]int }



in this secenerio, each widget type will have separated counter and e.g. buttons will not affect tab bar

3. another idea for auto id:
maybe we can make a `map[string]bool`.
now lets say in this reset sate mthod (can't remember its name, sorry) we clear the map
when we call `GenAutoID(id string)` it checks if given ID is in map already and if so it adds `##number` to it.
this will reduce chance of changing id of a widget with an internal state.
it also shouldn't affect performence because maps are relatively fast I think.
AllenDang commented 1 month ago

@gucio321 the 3rd solution is good!

gucio321 commented 1 month ago

ok, I'll add this as I get some time. btw @AllenDang should I revert this or I can leave it as is fore "more flexible api" :smile:

AllenDang commented 1 month ago

@gucio321 I my opinion, I'd like to keep GenAutoID stuff behind the scene, user doesn't need to know it, so I suggest to revert.

gucio321 commented 1 month ago

ok, done @AllenDang I've created a commit in https://github.com/AllenDang/giu/pull/797

Also, I think I can add some parts of the 2nd method especially, I think that highlighting when a string is ID and when it is just a label that will be proceeded by GenAutoID. WHat do you think?

AllenDang commented 1 month ago

@gucio321 Agree, sounds good.