Add Timer API #277
Add Timer API #277
Conversation
|
Thanks! This predicts what I was going to do eventually; good job. Are you sure the Thinking aloud section: I was planning on using In the // uiForEach represents the return value from one of libui's various ForEach functions.
_UI_ENUM(uiForEach) {
uiForEachContinue,
uiForEachStop,
};I wonder if this can be reused with some name changes somehow... Of course, the only other functions that could use that would be the |
Mac
Yes, from
WindowsIts not clear whether From MSDN:
(Also I wouldn't worry about higher precision timing at all, as you said, should probably use a thread!) |
|
Mac: ah, in that case then there is still a leak, as Windows: I don't mean "no window handle", I mean using the utility window (see |
MacWhoops, no ARC! The leak should be fixed now. WindowsI just pushed my first attempt at using |
|
I'll check the Windows documentation later and see if (and how) this can be improved. I was just reading the NSTimer documentation (for some unrelated reason) and I just realized something else we're overlooking:
If one of the other systems doesn't turn out to work like this, then we'll have programs that work slightly differently on different toolkits... Though again, this is just a documentation check, and |
|
If I'm understanding correctly, the most common scenario for a timer to be delayed like that would be to block the main UI thread for a significant period of time?? A situation that's always a bug in the application code.. Agreed we need to maintain the most consistent behavior across different toolkits as possible, and to document where those differences are unavoidable. The least "leaky abstraction" we can manage! |
|
So with my last two commits, the Windows code should now be on par with the other platforms. Any other issues with |
|
Timer IDs are pointer-sized, so we can get away with using the pointer to the timer metadata struct itself as an ID; should probably consider doing that instead. I can do that myself if you don't already have an idea of how to do so by the time I get back to this. Still trying to figure out repeating timer intervals though. I have a solution for GTK+, but not for Windows; MSDN isn't particularly helpful about this... Note to self: note that the |
Great idea, done!
What is the GTK+ solution? |
darwin/main.m
Outdated
| { | ||
| self = [super init]; | ||
| if (self) { | ||
| f = callback; |
There was a problem hiding this comment.
Always qualify ivars with self->.
There was a problem hiding this comment.
Good catch, will do.
examples/CMakeLists.txt
Outdated
| @@ -31,8 +31,14 @@ if(NOT WIN32) | |||
| target_link_libraries(cpp-multithread pthread) | |||
| endif() | |||
|
|
|||
| _add_example(timer | |||
There was a problem hiding this comment.
I wonder if the timer example should be merged with the cpp-multithread example...
There was a problem hiding this comment.
Ya timer was forked from cpp-multithread, trivial changes.
Merge them in what way? Single example program with a command-line switch??
There was a problem hiding this comment.
Single program with a button to choose whether to use a thread or uiTimer().
darwin/main.m
Outdated
| @@ -237,3 +237,44 @@ void uiQueueMain(void (*f)(void *data), void *data) | |||
| // the signature of f matches dispatch_function_t | |||
| dispatch_async_f(dispatch_get_main_queue(), data, f); | |||
| } | |||
|
|
|||
| @interface TimerDelegate : NSObject { | |||
There was a problem hiding this comment.
Also you might as well make this change now as I'm going to be making this change throughout libui at some point in the near future anyway: rename the class uiprivTimerDelegate. I know it makes code a bit less nice to read, but unless I move everything (except the public API) to C++ it's either reserve the uipriv prefix for private functions that get exposed to a public namespace (which includes all Objective-C classes) or keep the crazy cmake static library hacks that don't work. :/
There was a problem hiding this comment.
uipriv prefix works fine, will do. A bit verbose but totally unambiguous.
What cmake hacks are you referring to?
There was a problem hiding this comment.
The symbol stripping stuff on the GTK+ and macOS CMakeLists.txt and the resource file copying on Windows. Both have to do with quirks of static libraries, and the former is resolved by ensuring the namespace isn't polluted. (The latter is caused by a different issue, but the same problem applies.)
I only bring this up now because I already started doing this with utflib-and-attrstr.
There was a problem hiding this comment.
Oh the _handle_static macro??
Definitely want to get rid of that, breaks the build on OpenBSD! (#229)
There was a problem hiding this comment.
All the more reason to fix this! =P
unix/main.c
Outdated
| if((*(t->f))(t->data)) | ||
| return TRUE; | ||
| else { | ||
| g_free(t); |
There was a problem hiding this comment.
I'd suggest using uiNew() and uiFree() for these, for consistency. (I now wonder if I need to keep these around anyway...)
There was a problem hiding this comment.
I vote we keep uiNew() and uiFree(). Very useful for debugging, especially across the different platforms.
Also uiQueueMain() doesn't use them.. should it too? uiTimer() GTK+ implementation is based on it.
There was a problem hiding this comment.
uiQueueMain() doesn't use it because it is specifically intended for use across threads. Are you suggesting uiTimer() should also be usable across threads? Currently uiQueueMain() is the only function allowed to be used by other threads, and uiAlloc() and friends are not thread safe (by virtue of needing explicit initialization and deinitialization).
There was a problem hiding this comment.
No no, you're right. Nevermind!
In fact maybe uiQueueMain() should be more clearly documented as the only function in the API that isn't UI thread only.
|
Notes from my browser history: https://blogs.msdn.microsoft.com/oldnewthing/20141205-00/?p=43463 I don't think this has anything to do with this patch, but I'm putting it here anyway just for documentation purposes. |
|
I'm at the point where I'd like to merge this; would you mind addressing the existing review comments so I can do so? Thanks! |
| { | ||
| UINT_PTR timer; | ||
|
|
||
| timer = (UINT_PTR) new TimerHandler(f, data); |
There was a problem hiding this comment.
Are you sure you can use a pointer to an arbitrary structure here?
From what I understood of the win32 docs, if you give it both an hWnd and a nIDEvent arguments, the function expect it to point to an existing timer id linked to that window.
There was a problem hiding this comment.
Timer IDs are pointer-sized, so we can get away with using the pointer to the timer metadata struct itself as an ID; should probably consider doing that instead.
There was a problem hiding this comment.
Sorry I missed that comment... that's really an interesting design.
|
Oh I completely forgot about the timer IRC log stuff; disregard that other comment. I'll bring it out in a bit. |
|
Okay, here's the full IRC log: Make of this what you will. |
|
Interesting .. but I'm just going to ignore that for now honestly. Unless someone shows me a real application that breaks with that difference in behavior, it is out of scope for this PR. |
|
@andlabs Okay, |
|
While it may be out of scope for the initial implementation, it is not out of scope in general; I'll probably just make it a TODO for the documentation. Until then, will review now. Want to get this and the uiDatePicker stuff at least in before I continue symbol name cleanup. |
|
Most of my remaining comments are just stylistic nits and things related to the renaming, so I might as well just take care of them myself and take the load off you. Thanks a lot for doing this, and also for the uiDatePicker stuff! I do appreciate it! |
|
@andlabs You moved The build is currently broken. |
|
Should be fixed now; that was partially what I was referring to above =P |
|
Confirmed. The build is passing now! (Also- Its weird that GitHub markdown does not recognize @ usernames in the README..) |
|
It crashes on exit: How to correctly free timer? |
|
If I correctly understand the API, you have to return |
|
Yes, that is correct. Please file this as a separate issue; it's an issue with the example programs. |
First attempt at a basic timer API for libui. Still working on Windows implementation.