-
Notifications
You must be signed in to change notification settings - Fork 614
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
Table #343
Table #343
Conversation
also: fixed column-ordering bug changed file to hard tabs
Still a lot of features to exercise, but now provides some generated, multi-column data to work with.
windows expects a string to persist between WM_NOTIFY messages, so I've added one to the uiTable struct to keep it around and to clean up upon when the table is destroyed.
In LVN_GETDISPINFO, it turns out you can just fill out a provided text buffer rather than all the craziness involved with allocating a new one and making sure it hangs around long enough. Phew.
- enable full-row selection (instead of individual cell) - show hover tooltip when content too large for cell
mostly just stubs so far, barring some GTK+ support for multiselect
windows and mac just stubbed for now
Added a param to uiNewTable for passing in creation-time style flags, as we can't guarentee that all platforms support will support changing styles and behaviours on the fly. I'm looking at you, windows.
In target_compile_options(table PRIVATE --stdlib=libc++)
target_link_libraries(table --stdlib=libc++) At the moment, this is to silence a warning, but if you were to use some C++11 feature there would be a compile error. |
I thought I did that already. If I didn't, there's discussion about this somewhere that I'd want to point to in comments around those two lines... and it should go in its own commit with its own commit message (regardless of PR). |
I think a single uiTableModelAllChanged() function would be | ||
enough. The underlying control needs to know that it should | ||
to redraw everything visible (and the number of items might | ||
have chnaged). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted, the problem is GTK+ does not seem to provide a way to do this. There might be workarounds that are inefficient (requiring a signal emission for every row of the table, for instance), and I'm not sure if removing and re-adding the model will reset other settings like column widths and scroll positions. Will need to ask the GTK+ people about it.
Might want to note this here. Not required; not part of review.
All right. I appreciate everything being done here, but I decided to abandon my initial code review for two big reasons: a) the code formatting and style is very inconsistent with the rest of libui, so the review would have been flooded with stylistic consistency nitpicks, and I don't want to come off looking like a pedantic jerk, and b) and more important, this PR pushes in too many changes at once that I'm not fully comfortable with to just take in all at once, and we'd just be dragging this along forever if we discussed everything now all at once. It's my fault for not realizing just how much was going into this PR; sorry about that. But I don't want to reject it outright. Could we possibly split it into smaller PRs that add things a feature at a time? I can merge the (I will say that your Mac code is definitely wrong — your (I really need to find a way to nicely explain that libui code should also look the same as all the other code. I should investigate things like clang-format, maybe...) |
Sure - I'm happy to split it up into more easily-digestible chunks (and you can nit-pick them at leisure ;- ) I guess the rough plan would be:
Sound OK to you? |
Yeah. In fact, if the features are not interdependent on each other, we wouldn't have to go in a strict order either. I would do one feature first, though, just to get the general interface for features out of the way. Thanks again! |
👍 The test crashes on macOS when selecting a different row: $ ./out/test
2018-04-21 11:50:49.341 test[5045:322334] get 0x0
main window 0x7ffe21426320
size
Segmentation fault: 11
|
Thanks, @mischnic |
Is there an ETA on the new PR? I want to merge it before continuing my large-scale |
I'll aim to get the bare-bones comctl win32 implementation up for you in the next few days (ie no API changes, just adding windows support to the base table branch) It'll incorporate #287, too, I'm afraid, as I need that to compile. I assume you can skip those commits in the PR, but let me know if there's a more elegant approach. |
(see #310)
For unix/darwin/windows, this adds support for:
The windows version supports only text columns (being based on the rather limited win32 commctl listview).