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

Column width data is no longer lost when dragged to the right side of the window #1499

Closed
wants to merge 1 commit into from

Conversation

ggtucker
Copy link
Contributor

@ggtucker ggtucker commented Dec 13, 2017

Fixed an issue where dragging the columns to the edge of the window would collapse the columns (since they are now forced within the window) and their width data would be lost. With this change, the old column widths are remembered until the columns are no longer being dragged. This means columns can be collapsed by dragging them all the way to the right side of the window, and then expanded again by dragging them back to the left, without losing their width data until the columns are actually deselected and no longer being dragged.

I'm not super happy with how the column state is being stored in the StateStorage object, but this was the quickest fix I could put in using the existing tech.

With this change, this is how the column state storage looks:

  • StateStorage(ColumnsSetId - 1) The boolean state for whether the columns were dragging last update
  • StateStorage(ColumnsSetId + 2 * ColumnIndex) The float for the original column offset norm
  • StateStorage(ColumnsSetId + 2 * ColumnIndex + 1) The float for the dragged column offset norm

…ould collapse the columns, losing their width data. With this change, the old column widths are remembered until the columns are no longer being dragged. This means columns can be collapsed by dragging them all the way to the right side of the window, and then expanded again by dragging them back to the left, without losing their width data until the columns are actually deselected and no longer being dragged.
@ocornut
Copy link
Owner

ocornut commented Dec 13, 2017

Geoffrey,

I understand the problem, thought I think maybe we should bite the bullet and reorganize the columns code to store data in dedicated structures. The main reason it is using StateStorage instead of ColumnsData[] right now is because we can have multiple column sets, so ColumnsData[] is just transient storage. But it's a little silly right now to keep that system.

I'll try to do this refactor at some point, have e.g. a ImGuiColumnsData structure with all the columns state, keyed by main nColumns() id.

Omar

@ggtucker
Copy link
Contributor Author

Yeah that's understandable. If I find the time I may try exploring the ImGuiColumnsData storage myself and see where it takes us. Thanks for the quick response!

@ocornut
Copy link
Owner

ocornut commented Dec 13, 2017

I actually started working on refactoring those structures just now, so will probably push something shortly with the cleanup, and your feature will be much simpler to implement then.

@ggtucker
Copy link
Contributor Author

Ooh awesome, thanks Omar! :D

ocornut added a commit that referenced this pull request Dec 13, 2017
ocornut added a commit that referenced this pull request Dec 13, 2017
… persistent data for most + fix for pre C++11 compilers. (#125, #1499)
ocornut added a commit that referenced this pull request Dec 13, 2017
@ocornut
Copy link
Owner

ocornut commented Dec 13, 2017

Geoffrey, this is now pushed (commit ddbcda8 which you'll notice is now much simpler). I also reorganized the data structures so working on columns will be easier onward.
(if you had other columns patches they will probably conflict, sorry)

@ocornut ocornut closed this Dec 13, 2017
@ggtucker
Copy link
Contributor Author

Looks great!

ocornut added a commit that referenced this pull request Mar 4, 2018
ocornut added a commit that referenced this pull request Mar 4, 2018
…p introduced by #913 was ok as it didn't write back into the storage, which #1499 started doing making it destructive. Right now I don't think the clamp is needed at all. It had uses (eg: hide the issue fixed by bf7481e).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants