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

add GenAutoID to Context #795

Merged
merged 1 commit into from
May 8, 2024
Merged

add GenAutoID to Context #795

merged 1 commit into from
May 8, 2024

Conversation

gucio321
Copy link
Collaborator

@gucio321 gucio321 commented May 8, 2024

this allows to disable GenAutoID

@gucio321 gucio321 linked an issue May 8, 2024 that may be closed by this pull request
@gucio321 gucio321 merged commit 58bd6f8 into AllenDang:master May 8, 2024
4 checks passed
@gucio321 gucio321 deleted the auto-id branch May 8, 2024 07:43
@AllenDang
Copy link
Owner

@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
Copy link
Owner

@gucio321 To globally disable the GenAutoID is not necessary.

@gucio321
Copy link
Collaborator Author

gucio321 commented May 9, 2024

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
Copy link
Owner

@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
Copy link
Collaborator Author

gucio321 commented May 9, 2024

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

  1. 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
Copy link
Owner

@gucio321 the 3rd solution is good!

@gucio321
Copy link
Collaborator Author

gucio321 commented May 9, 2024

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" 😄

@AllenDang
Copy link
Owner

@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
Copy link
Collaborator Author

gucio321 commented May 9, 2024

ok, done
@AllenDang I've created a commit in #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
Copy link
Owner

@gucio321 Agree, sounds good.

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

Successfully merging this pull request may close these issues.

GenAutoID: add possibility to disable it
2 participants