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

added new method set_window_icon for macos, windows, html5 platform #71

Merged
merged 5 commits into from Mar 6, 2018

Conversation

Projects
None yet
3 participants
@AGulev
Copy link
Collaborator

commented Feb 28, 2018

No description provided.

@AGulev AGulev requested review from subsoap and dapetcu21 Feb 28, 2018

@subsoap
Copy link
Owner

left a comment

Tested for bundled Windows and HTML5

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

Why not accept an absolute path into that function? (on macOS) That way it can be consistent with set_mouse_cursor (and could be made to work when not bundled).

What's really missing, though, is a function to get the absolute path to the bundle root. I made something similar here: https://github.com/dapetcu21/defold-fmod/blob/master/bridge/src/fmod_dynamic_loading.cpp#L76

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

I see that for HTML5 and Windows you're passing the path directly and it would be relative to the current directory, which is not guaranteed to be the same as the directory containing your executable.

@AGulev

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2018

I saw how did you work with cursor, and I don't' want to work this way with icon.
Icon it's something static. Something that developer want to have when game started.

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

Yes, yet it's confusing to be using absolute paths in one place and relative paths in another place. Furthermore, on Windows the path is not even relative to the right thing. You want it to be relative to the game's .exe, not to the current directory.

@AGulev

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2018

how do you propose to fix that?

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

Add a get_bundle_root() function that would get you the absolute path to the:

  • directory containing the .exe on Windows
  • .app bundle on macOS

(see what I linked above)

@AGulev

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2018

I like this solution, I'll implement it soon

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

You should also add a defos.PATH_SEP that is set to "\" on Windows and "/" everywhere else so that users can do
defos.get_bundle_root() .. defos.PATH_SEP .. icons[system_name]

@AGulev

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2018

I can return path already with separator

void defos_set_window_icon(const char *icon_path)
{
NSString *bundlePath = [[NSBundle mainBundle] resourcePath];
NSString *secondParentPath = [[bundlePath stringByDeletingLastPathComponent] stringByDeletingLastPathComponent];

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Feb 28, 2018

Collaborator

You could have gotten to this in a more easier way:

[[NSBundle mainBundle] bundlePath]

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 1, 2018

Author Collaborator

ok, thx.

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

Please don't return it with separator. The user might want to use it for something else as well. And they might want to compose their own path, so having the separator separately is helpful.

@AGulev

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2018

@dapetcu21 done.
Maybe you know why I receive an error:
2018-03-04_19-44-59
when I try to use free() method?
#include <stdlib.h> - didn't help

@dapetcu21

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2018

free() won't work in that context because you're trying to free a const char*, which cannot be converted to void *, which is what free() wants. You need to drop the const.

document.head || (document.head = document.getElementsByTagName('head')[0]);
function changeFavicon(src) {
var link = document.createElement('link');
var oldLink = document.getElementById('dynamic-favicon');

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Mar 6, 2018

Collaborator

If the user has a <link rel='shortcut icon'> already in the document that wasn't inserted by defos, he'll end up having two <link>'s. Maybe do var oldLink = document.querySelector('link[rel="shortcut icon"]')?

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

done

stringToUTF8(jsString, stringOnWasmHeap, lengthBytes+1);
return stringOnWasmHeap;
},0);
return bundlePath;

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Mar 6, 2018

Collaborator

This leaks the bundlePath string after it gets passed to Lua in get_bundle_root(). Maybe always make sure we return a string we own and free() it in get_bundle_root()?

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

I wanted to do this (I asked you aboutt free()), it was my fault with const, thx!

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

done

void defos_set_window_icon(const char *icon_path)
{
NSString *path = [NSString stringWithUTF8String:icon_path];
NSImage* image = [[NSImage alloc] initWithContentsOfFile: path];

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Mar 6, 2018

Collaborator

This image leaks. It needs to be released after giving it to the window button.

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

done

char const* defos_get_bundle_root() {
char bundlePath[MAX_PATH];
GetCurrentDirectory(MAX_PATH, bundlePath);
return bundlePath;

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Mar 6, 2018

Collaborator

You're returning a reference to a string on the stack, which will be invalid memory after this function returns. Allocate memory for the string and make sure it gets freed after passing it to Lua

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

done

@@ -93,6 +93,20 @@ void defos_set_window_title(const char* title_lua) {
[window setTitle:title];
}

void defos_set_window_icon(const char *icon_path)
{
NSString *path = [NSString stringWithUTF8String:icon_path];

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Mar 6, 2018

Collaborator

This string gets created and autoreleased, meaning it will get garbage collected by the nearest autorelease pool. We don't know how Defold works internally, so we don't know if it has an autorelease pool in all contexts this function might get called. It would be safest to surround this with an @autorelease { ... }.

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

ok, i'll do this. thx.
I think Defold has no autorelease pool in all context, i found this issue with other my NE :
https://forum.defold.com/t/native-extensions/4946/149?u=agulev

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

done

char const* defos_get_bundle_root() {
NSString *bundlePath = [[NSBundle mainBundle] bundlePath];
const char *bundlePath_lua = [bundlePath UTF8String];
return bundlePath_lua;

This comment has been minimized.

Copy link
@dapetcu21

dapetcu21 Mar 6, 2018

Collaborator

This is fine. It won't leak since it's owned by the bundle, but if we add a free() in get_bundle_root() as I suggested for HTML and Windows, we should copy this string into some memory we own.

This comment has been minimized.

Copy link
@AGulev

AGulev Mar 6, 2018

Author Collaborator

done

AGulev added some commits Mar 6, 2018

memory leak bug fixing
try to find <link> in html5 realisation before creation of new icon

@AGulev AGulev merged commit 1a3a4a4 into subsoap:master Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.