Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

robert-ancell
Copy link
Contributor

FlValue is a lightweight object used to contain the value types that Flutter
uses on platform channels.

@robert-ancell
Copy link
Contributor Author

There's some discussion in #18118 about how custom data types would be represented in a FlValue (in a following PR).

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

A few small things, but generally looks good if we keep this structure.

One high-level question though (which I think you may have touched on in the other PR, but since I'm not familiar with all the GLib APIs I'm not sure): how much overhead would it add to use existing (and I assume standard?) collection types like garray and g_hash_table? One of the design goals of the C++ verison of this class was to use standard library collections so that people could use all the normal collection APIs they are already familiar with, instead of having to learn a new, more limited API surface, so I'm wondering how feasible that would be here.

@robert-ancell robert-ancell force-pushed the linux-shell-fl-value branch from 40bf39e to a67bb89 Compare May 7, 2020 21:49
@robert-ancell
Copy link
Contributor Author

The two main used data structures in GLib are GPtrArray (list) and GHashTable (map). They both don't have any type information and just use pointers for the data. Often the data is an object with an unref function provided. Because there's no type safety you generally only see these used for output (a docstring gives the element-type inside these). Using them for input is risky as the user has to get both the type correct and set the appropriate destroy notify (or not). The library can't handle any errors in these cases.

They're also a pain to make, the equivalent code would be:

// With FlValue native

g_autoptr(FlValue) list = fl_value_new_list();
fl_value_list_append_take(list, fl_value_new_string ("foo"));
fl_value_list_append_take(list, fl_value_new_int (42));

g_autoptr(FlValue) map = fl_value_new_map();
fl_value_map_set_take(list, fl_value_new("name"), fl_value_new_string ("foo"));
fl_value_map_set_take(list, fl_value_new("value"), fl_value_new_int ("42"));

// With GPtrArray/GHashTable

g_autoptr(GPtrArray) list = g_ptr_array_new_with_free_func(fl_value_unref);
g_ptr_array_add(list, fl_value_new_string ("foo"));
g_ptr_array_add(list, fl_value_new_int (42));
g_autoptr(FlValue) value = fl_value_new_list(list);

g_autoptr(GHashTable) map = g_hash_table_new_full(g_direct_hash, g_direct_equal, fl_value_unref, fl_value_unef);
g_hash_table_replace(map, fl_value_new("name"), fl_value_new_string ("foo"));
g_hash_table_replace(map, fl_value_new("value"), fl_value_new_int ("42"));
g_autoptr(FlValue) value = fl_value_new_map(map);

@robert-ancell
Copy link
Contributor Author

I did consider helper functions to build lists and maps from GPtrArray and GHashTable but I honestly don't think there'd be much demand for them and the risk of encouraging programming errors is too great. We could always add them later if required. We could consider adding a GVariant to FlValue conversion (which is provided in Json GLib for example), but again I think the programmer error risk outweighs the value.

@robert-ancell
Copy link
Contributor Author

I made one final change to the names of the functions, e.g. fl_value_new_int is now fl_value_int_new and fl_value_map_set is now fl_value_set. This is because I was originally thinking of exposing different structures for each subtype of FlValue, but that is no longer the case. The naming is important as it allows language bindings that have constructors to represent this as FlValue.int(42).

@robert-ancell robert-ancell force-pushed the linux-shell-fl-value branch from a67bb89 to 501f575 Compare May 7, 2020 22:18
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

The two main used data structures in GLib are GPtrArray (list) and GHashTable (map). They both don't have any type information and just use pointers for the data. Often the data is an object with an unref function provided. Because there's no type safety you generally only see these used for output (a docstring gives the element-type inside these). Using them for input is risky as the user has to get both the type correct and set the appropriate destroy notify (or not). The library can't handle any errors in these cases.

Okay, makes sense then. I was assuming there would be something ubiquitous (more along the lines of std::vector) that it would be annoying not to interoperate cleanly with, but if that's not the case this looks good.

FlValue is a lightweight object used to contain the value types that Flutter
uses on platform channels.
@robert-ancell robert-ancell force-pushed the linux-shell-fl-value branch from 501f575 to db9e07e Compare May 7, 2020 23:14
@robert-ancell robert-ancell merged commit 403931f into flutter:master May 8, 2020
@robert-ancell robert-ancell deleted the linux-shell-fl-value branch May 8, 2020 01:48
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2020
git@github.com:flutter/engine.git/compare/3953c3ccd15a...403931f

git log 3953c3c..403931f --first-parent --oneline
2020-05-08 robert.ancell@canonical.com Add FlValue (flutter/engine#18185)
2020-05-08 jason-simmons@users.noreply.github.com [SkParagraph] Copy text height behavior to the Skia paragraph style (flutter/engine#18178)
2020-05-08 ferhat@gmail.com [web] Implement matrix parameter for linear gradient (flutter/engine#18208)
2020-05-08 liyuqian@google.com Reland again "Remove layer integral offset snapping flutter#17112" (flutter/engine#18160)
2020-05-08 skia-flutter-autoroll@skia.org Roll src/third_party/skia c66cd987f7c0..0dc207f836da (5 commits) (flutter/engine#18212)
2020-05-08 stuartmorgan@google.com Refactor GLFW embedding to support headless mode (flutter/engine#18205)
2020-05-07 a-siva@users.noreply.github.com Manual Roll of Dart 39e0e75fcf...ce62ad2e8b (flutter/engine#18211)
2020-05-07 38722979+Kurun-pan@users.noreply.github.com Support EventChannel C++ plugin API for Linux/Windows (flutter/engine#17015)
2020-05-07 robert.ancell@canonical.com Handle leak of message handle when no engine present (flutter/engine#18157)
2020-05-07 xster@google.com add docs to platformviewios (and some drive-by changes) (flutter/engine#17593)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0066adefa97d..c66cd987f7c0 (4 commits) (flutter/engine#18206)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia edea19858ccc..0066adefa97d (3 commits) (flutter/engine#18203)
2020-05-07 jason-simmons@users.noreply.github.com Remove the global engine entry timestamp (flutter/engine#18182)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/dart e86e4d61834a..ce62ad2e8b40 (13 commits) (flutter/engine#18201)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 2871ab0727bf..edea19858ccc (3 commits) (flutter/engine#18198)
2020-05-07 dtiselice@google.com Fixed ChildSceneLayer elevation issue on Fuchsia. (flutter/engine#18144)
2020-05-07 iska.kaushik@gmail.com [profiling] CPU Profiling support for iOS (flutter/engine#18087)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia e3d1de7c5281..2871ab0727bf (1 commits) (flutter/engine#18197)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/dart 733153eb517c..e86e4d61834a (6 commits) (flutter/engine#18195)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3b2db26c59d6..e3d1de7c5281 (4 commits) (flutter/engine#18192)
2020-05-07 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from jMJqf... to 1MVsE... (flutter/engine#18194)
2020-05-07 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from RpHTv... to MhpFP... (flutter/engine#18191)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 88d04cb51acf..3b2db26c59d6 (1 commits) (flutter/engine#18190)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/dart 4da5b40fb6dc..733153eb517c (23 commits) (flutter/engine#18188)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC garyq@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2020
git@github.com:flutter/engine.git/compare/3953c3ccd15a...9193d8f

git log 3953c3c..9193d8f --first-parent --oneline
2020-05-08 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0dc207f836da..a14084ba1b41 (3 commits) (flutter/engine#18219)
2020-05-08 robert.ancell@canonical.com Add FlValue (flutter/engine#18185)
2020-05-08 jason-simmons@users.noreply.github.com [SkParagraph] Copy text height behavior to the Skia paragraph style (flutter/engine#18178)
2020-05-08 ferhat@gmail.com [web] Implement matrix parameter for linear gradient (flutter/engine#18208)
2020-05-08 liyuqian@google.com Reland again "Remove layer integral offset snapping flutter#17112" (flutter/engine#18160)
2020-05-08 skia-flutter-autoroll@skia.org Roll src/third_party/skia c66cd987f7c0..0dc207f836da (5 commits) (flutter/engine#18212)
2020-05-08 stuartmorgan@google.com Refactor GLFW embedding to support headless mode (flutter/engine#18205)
2020-05-07 a-siva@users.noreply.github.com Manual Roll of Dart 39e0e75fcf...ce62ad2e8b (flutter/engine#18211)
2020-05-07 38722979+Kurun-pan@users.noreply.github.com Support EventChannel C++ plugin API for Linux/Windows (flutter/engine#17015)
2020-05-07 robert.ancell@canonical.com Handle leak of message handle when no engine present (flutter/engine#18157)
2020-05-07 xster@google.com add docs to platformviewios (and some drive-by changes) (flutter/engine#17593)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0066adefa97d..c66cd987f7c0 (4 commits) (flutter/engine#18206)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia edea19858ccc..0066adefa97d (3 commits) (flutter/engine#18203)
2020-05-07 jason-simmons@users.noreply.github.com Remove the global engine entry timestamp (flutter/engine#18182)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/dart e86e4d61834a..ce62ad2e8b40 (13 commits) (flutter/engine#18201)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 2871ab0727bf..edea19858ccc (3 commits) (flutter/engine#18198)
2020-05-07 dtiselice@google.com Fixed ChildSceneLayer elevation issue on Fuchsia. (flutter/engine#18144)
2020-05-07 iska.kaushik@gmail.com [profiling] CPU Profiling support for iOS (flutter/engine#18087)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia e3d1de7c5281..2871ab0727bf (1 commits) (flutter/engine#18197)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/dart 733153eb517c..e86e4d61834a (6 commits) (flutter/engine#18195)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3b2db26c59d6..e3d1de7c5281 (4 commits) (flutter/engine#18192)
2020-05-07 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from jMJqf... to 1MVsE... (flutter/engine#18194)
2020-05-07 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from RpHTv... to MhpFP... (flutter/engine#18191)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/skia 88d04cb51acf..3b2db26c59d6 (1 commits) (flutter/engine#18190)
2020-05-07 skia-flutter-autoroll@skia.org Roll src/third_party/dart 4da5b40fb6dc..733153eb517c (23 commits) (flutter/engine#18188)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC garyq@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants