Skip to content

Commit

Permalink
cocoa headers
Browse files Browse the repository at this point in the history
  • Loading branch information
@parro-it
parro-it committed Apr 15, 2018
1 parent 5dbd532 commit bcbe5c1
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/arch/darwin/timer.mm
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "timer.h"
#include "nbind/api.h"
#import <Cocoa/Cocoa.h>
#import <CoreFoundation/CoreFoundation.h>

TimeoutHandle *setTimeout(nbind::cbFunction &cb, unsigned int timeout) {
nbind::cbFunction *callbackJs = new nbind::cbFunction(cb);
Expand Down

15 comments on commit bcbe5c1

@mischnic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only Cocoa is needed, but this needs to be imported in timer.h because of

typedef NSTimer *TIMER_HANDLE;

However, timer.cc includes that file and importing Objective-C headers in an C++ file doesn't work.

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

Just discovered :-(

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

I switched to void*

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

It compile on Travis... Could you check if it's working on macOS?

@mischnic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Works.
What do you think about replacing the default functions?

index.js:

global.setTimeout = binding.lib.setTimeout;
global.clearTimeout = binding.lib.clearTimeout;
global.setInterval = binding.lib.setInterval;
global.clearInterval = binding.lib.clearInterval;

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

What do you think about replacing the default functions?

Yes, indeed. But the current functions does not fully respect the node API: setInterval and setTiemout need to accept variadic arguments after the timeout, that they pass to the callback evrytime is called.

Since nbind does not support yet variadic arguments, I thought of add an array argument on C++ side, and then on JS side convert rest arguments to array, and pass that to the C++ functions.

@mischnic
Copy link
Collaborator

@mischnic mischnic commented on bcbe5c1 Apr 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This seems to do the trick.

global.setTimeout = (func, t, ...args) => {
	return binding.lib.setTimeout(() => {
		func(...args);
	}, t);
};
global.clearTimeout = binding.lib.clearTimeout;
global.setInterval = (func, t, ...args) => {
	return binding.lib.setInterval(() => {
		func(...args);
	}, t);
};
global.clearInterval = binding.lib.clearInterval;

@parro-it
Copy link
Owner Author

@parro-it parro-it commented on bcbe5c1 Apr 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure about node version compatibility

From here https://node.green/ it seems from node 6 onwards.
We could do:

global.setTimeout = (func, t /*...args*/) => {
	return binding.lib.setTimeout(funct, t, Array.prototype.slice.call(arguments,2));
};

@mischnic
Copy link
Collaborator

@mischnic mischnic commented on bcbe5c1 Apr 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This would work without any C++ changes:

global.setTimeout = function(cb, t) {
	const args = Array.prototype.slice.call(arguments, 2);
	return binding.lib.setTimeout(function() {
		cb.apply(null, args);
	}, t);
};

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

And also, we should somehow patch the globals only after startLoop, and restore the original after stopLoop,
because our timers need the GUI event loop to be running to work.

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

It's working on windows too...

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

Sadly, the clang-format is not working on windows, we should at least add a note somewhere on README or documentation.

@mischnic
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would work without any C++ changes:

And also, we should somehow patch the globals only after startLoop, and restore the original after stopLoop

See e6387d8

@mischnic
Copy link
Collaborator

@mischnic mischnic commented on bcbe5c1 Apr 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sadly, the clang-format is not working on windows, we should at least add a note somewhere on README or documentation.

How exactly? Related to this: ? angular/clang-format#25

@parro-it
Copy link
Owner Author

Choose a reason for hiding this comment

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

How exactly? Related to this: ? angular/clang-format#25

Nevermind, I didn't install it from http://llvm.org/builds/ as specified in clang-format docs...

Please sign in to comment.