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.

Sign up for GitHub

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

Jump to bottom

fix undefined value not converted to gobject #334

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

fix undefined value not converted to gobject #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23d6474
fix undefined value not converted to gobject
CapitaineJSparrow Oct 4, 2022
cb9e667
add unit test
CapitaineJSparrow Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,8 +1368,15 @@ void FreeGIArgumentArray(GITypeInfo *type_info, GIArgument *arg, GITransfer tran
*/

bool CanConvertV8ToGValue(GValue *gvalue, Local<Value> value) {
auto maybeDetailString = Nan::ToDetailString(value);
Nan::Utf8String utf8String(
!maybeDetailString.IsEmpty() ?
maybeDetailString.ToLocalChecked() :
UTF8("[invalid value]")
);

// void/null
if (G_VALUE_TYPE(gvalue) == G_TYPE_INVALID)
if (G_VALUE_TYPE(gvalue) == G_TYPE_INVALID || strcmp(*utf8String,"undefined") == 0)
return value->IsNullOrUndefined();
Comment on lines +1371 to 1380
Copy link
Owner

Choose a reason for hiding this comment

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

This check seems a bit more complex than it needs to be. The last line here already tells you how to check if a value is undefined, but you could only check for undefined with value->IsUndefined(). Which is way better because this function is called very often and we don't want a string comparisons to be run for each call to this function.

Could you also tell me a bit more about the root cause of #333? Reason is, this function checks if we can convert a V8 value into a GValue's type. But if we add a check if (... || value->IsUndefined()) return value->IsNullOrUndefined(), then we're returning true for any call to this where value === undefined, even if the GType we want to convert to isn't void.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry I don't have enough knowledge to tell you the root cause of #333 Consider my PR as a bug report I don't understand a lot about V8 and how the library works internally, neither cpp code. The only interesting part in my PR is the unit test. I'm marking this PR as draft.

But if we add a check if (... || value->IsUndefined()) return value->IsNullOrUndefined(), then we're returning true for any call to this where value === undefined, even if the GType we want to convert to isn't void.

You are completely right here, do you see a better approach to manage this case ?


if (G_VALUE_HOLDS_BOOLEAN (gvalue)) {
Expand Down Expand Up @@ -1426,8 +1433,7 @@ bool V8ToGValue(GValue *gvalue, Local<Value> value, bool mustCopy) {
maybeDetailString.ToLocalChecked() :
UTF8("[invalid value]")
);
Throw::TypeError("Cannot convert value \"%s\" to type %s",
*utf8String, G_VALUE_TYPE_NAME (gvalue));
Throw::TypeError("Cannot convert value \"%s\" to type %s",*utf8String, G_VALUE_TYPE_NAME (gvalue));
return false;
}

Expand Down
13 changes: 13 additions & 0 deletions tests/signal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,19 @@ window.on('show', () => {
)
})

describe('Trigger "on-focus-out-event" should work', async () => new Promise((resolve, reject) => {
const searchEntry = new Gtk.SearchEntry();
const event = new Gdk.EventFocus()
event.type = Gdk.EventType.FOCUS_CHANGE
event.window = searchEntry.getWindow()
event.sendEvent = 1

searchEntry.on('focus-out-event', (event) => resolve())
searchEntry.emit('focus-out-event', event);
// In case resolve is never called, do not block test indefinitely
setTimeout(() => reject(), 1000);
}));

describe('types are as correct as possible', () => {
const event = new Gdk.EventButton()
event.type = Gdk.EventType.BUTTON_PRESS
Expand Down