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

Feat permissive dropping, in which i suffer #440

Merged
merged 7 commits into from Mar 1, 2021
Merged

Feat permissive dropping, in which i suffer #440

merged 7 commits into from Mar 1, 2021

Conversation

sanbox-irl
Copy link
Member

  • Most tokens through the repository (eg. WindowToken, TabBarToken, FontStackToken, etc) now allow for permissive dropping -- i.e, you don't need to actually call the .end() method on them anymore. In exchange, these tokens have taken on a lifetime, which allows them to be safe.

  • end() no longer takes Ui. This is a breaking change, but hopefully should be trivial (and perhaps nice) for users to fix. Simply delete the parameters, or add a _ before the token's binding name and allow it to be dropped on its own.

  • PopupModal's new was reworked so that it didn't take Ui until build was called. This is a breaking change if you were invoking it directly. Simply move your ui call to build or begin.

I've changed my mind and think we shouldn't merge this till after 0.7 drops. Although the breaking changes this PR introduces are trivial, they can be substantial in a larger project. In our examples, it was about 23 single token changes (generally just removing an argument), and in my own private project, it was about 53, equally trivial changes.

I've also decided to leave all the pop methods in place. They just call end (which is an empty method just to invoke drop), but since ImGui uses pop sometimes for these methods, I think it's reasonable to have pop for people looking at ImGui C++, and end for people used to seeing the same "end" method in imgui-rs (although I'd say the idiomatic thing is just to _bind the variable and let it drop on its own now).

A few tokens survived the purge. A very used one (TreeNode), I manually implemented -- we need to make sure to not drop it if a certain flag was passed in. For ListClipperToken, I made an issue because I have no idea what that is and should probably learn #438, and for a few style tokens, they didn't have required drops on them right now. That might have been an oversight that exists, but I'll look into it eventually. #439

I would also like some golf claps for all of these edits and an acknowledgement of the suffering of editing all of these files, thank you.

@sanbox-irl sanbox-irl mentioned this pull request Feb 4, 2021
@thomcc
Copy link
Member

thomcc commented Feb 28, 2021

There's nothing blocking landing this right?

@sanbox-irl
Copy link
Member Author

We'd talked about a totally different theory around this involving RCs, rather than just making it more permissive

@sanbox-irl
Copy link
Member Author

Nothing stopping landing this and then doing that later though

@thomcc
Copy link
Member

thomcc commented Feb 28, 2021

Hmm, yeah, that's compelling, especially since IIUC you're using this branch at work, which means to me it probably works well enough.

I'd rather land it and fix it up later rather then have it bitrot.

@sanbox-irl
Copy link
Member Author

👍 i got the time to day to fix it up and merge it in with a changelog.

hoooo boy lol, 0.8 for us is going to be a "welcome to the new maintainers" release. Looking forward to seeing if people are pleased with the changes, even with all the breakages

@thomcc
Copy link
Member

thomcc commented Feb 28, 2021

hoooo boy lol, 0.8 for us is going to be a "welcome to the new maintainers" release. Looking forward to seeing if people are pleased with the changes, even with all the breakages

I think we should also do the string stuff. Should have some time this week to work on it.

@sanbox-irl
Copy link
Member Author

Perfect. this is gonna be good

@thomcc
Copy link
Member

thomcc commented Mar 1, 2021

It might be worth writing the changelog for this, but if you want I can merge it and we can catch up with that later.

@sanbox-irl
Copy link
Member Author

Will be done with it in an hour or so. Just out on a run -- so I can write the change log too. I'm in a very markdown mood

@thomcc
Copy link
Member

thomcc commented Mar 1, 2021

Literally no urgency at all. No worries.

@sanbox-irl
Copy link
Member Author

rebasing, how does it work, no one knows. please ignore this messy history I am smacking it down and making it beautiful

@sanbox-irl
Copy link
Member Author

Alright, merging! Godspeed to everyone in fixing any breaking changes. There will be some advice in the CHANGELOG on how to do that.

@sanbox-irl sanbox-irl merged commit 75fdec0 into imgui-rs:master Mar 1, 2021
@sanbox-irl sanbox-irl mentioned this pull request Mar 9, 2021
4 tasks
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.

None yet

2 participants