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

[Old PR libui#512] Add new API functions to get and set the visibility of table headers. #23

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

szanni
Copy link
Contributor

@szanni szanni commented Jan 14, 2022

Old PR libui#512

Proposal for two new API additions to un/hide table headers:

int uiTableHeaderVisible(uiTable *t);
void uiTableHeaderSetVisible(uiTable *t, int visible);

This was one of the requests in the table master issue.

--

Added API functions:
uiTableHeaderVisible() to determine whether the table header is visible.
uiTableHeaderSetVisible() to set the visibility of the table header.

Implementation provided for unix, darwin, and windows.

Notes: as darwin does not provide an API for hiding or recreating the
table header I opted for saving a reference and restoring that when the
visibility is set back to true. Setting the header to nil to hide it is
the suggested method for hiding the header according to the docs.

Added API functions:
uiTableHeaderVisible() to determine whether the table header is visible.
uiTableHeaderSetVisible() to set the visibility of the table header.

Implementation provided for unix, darwin, and windows.

Notes: as darwin does not provide an API for hiding or recreating the
table header I opted for saving a reference and restoring that when the
visibility is set back to true. Setting the header to nil to hide it is
the suggested method for hiding the header according to the docs.
@cody271 cody271 added the old repo original andlabs/libui project label Jan 15, 2022
Copy link
Contributor

@cody271 cody271 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows: LVM_GETHEADER message appears to not set an error, so not much to do besides report "unknown error" back to the user from uiTableHeaderVisible(). Also try to match the existing style of explicitly sending a WM message, instead of using the wrapper. The wrappers are less portable across different Windows toolchains.

windows/table.cpp Outdated Show resolved Hide resolved
windows/table.cpp Outdated Show resolved Hide resolved
@cody271
Copy link
Contributor

cody271 commented Jan 16, 2022

Also wrapper macros here

@szanni
Copy link
Contributor Author

szanni commented Jan 16, 2022

windows: LVM_GETHEADER message appears to not set an error, so not much to do besides report "unknown error" back to the user from uiTableHeaderVisible(). Also try to match the existing style of explicitly sending a WM message, instead of using the wrapper. The wrappers are less portable across different Windows toolchains.

I did not know the macros were less portable. Any reference for that? Because the WM messages look hideous to me. So much clutter and risk of introducing bugs that I would personally love to avoid.
I was actually going to suggest enforcing the use of the wrapper macros for newer code.

@cody271
Copy link
Contributor

cody271 commented Jan 17, 2022

I did not know the macros were less portable. Any reference for that?

Hrmm the references I'm finding to this issue are very old. I would hope it is fixed by now, decade(s) later.
Some references to this issue in our windows/ code already here.

@cody271
Copy link
Contributor

cody271 commented Jan 17, 2022

Because the WM messages look hideous to me. So much clutter and risk of introducing bugs that I would personally love to avoid. I was actually going to suggest enforcing the use of the wrapper macros for newer code.

You are not wrong, it is hideous. Regardless though, the more important point is that we should stay as consistent as possible with all the existing conventions. When we do adopt the macro wrappers over sending WM messages, it needs to be all or nothing across the entire windows/ codebase.

@szanni
Copy link
Contributor Author

szanni commented Jan 17, 2022

Hrmm the references I'm finding to this issue are very old. I would hope it is fixed by now, decade(s) later. Some references to this issue in our windows/ code already here.

That does indeed look old and seems to refers to mingw, which to my knowledge has always been very incomplete. I am pretty sure that's why only mingw-w64 is listed as supported in the first place.

You are not wrong, it is hideous. Regardless though, the more important point is that we should stay as consistent as possible with all the existing conventions. When we do adopt the macro wrappers over sending WM messages, it needs to be all or nothing across the entire windows/ codebase.

Ok. I can change things to use SendMessageW. My idea was to use macros in new code while leaving SendMessageW in old code so we don't get too many merge conflicts.
I guess changing things could be done at a later stage though, just to make sure we don't break anything in the fork for the time being at least.
It is just code like this that seems very unsafe to me and rife for bugs.

@cody271
Copy link
Contributor

cody271 commented Jan 18, 2022

I guess changing things could be done at a later stage though, just to make sure we don't break anything in the fork for the time being at least.
It is just code like this that seems very unsafe to me and rife for bugs.

Issue to fix this here

Copy link
Contributor

@cody271 cody271 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cody271 cody271 merged commit 5aa86da into libui-ng:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old repo original andlabs/libui project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants