Here is an attempt to fix this issue. Any feedbacks appreciated.

Created attachment 335641 [details] [review]
patch proposal v1

Created attachment 335652 [details] [review]
patch proposal v2

Review of attachment 335652 [details] [review]:

This is a good first cut - the general approach is sound, there's just some details that need sorting out.

::: CMakeLists.txt
@@ +63,1 @@
 

I'm not sure if this is something we want to make a compile time option.

When compiling against GTK+ < 3.20, we have to use GtkFileChooserDialog since the native one isn't available, hence the compile time option is redundant.

When compiling against GTK+ >= 3.20 we do have the option, but we should just chose one and then use that. I think it's reasonable to always use GtkFileChooserNative in this case, since it's one less avenue for bugs to creep in, and one less thing to remember to ask about when debugging problems.

@@ +87,3 @@
 
+pkg_check_modules(GTK3 QUIET gtk+-3.0>=3.14.0)
+

We already have this check in src/CMakeLists.txt - do we need it twice? If so, the desired minimum version should be set as a variable and used both here and there.

::: src/CMakeLists.txt
@@ +457,3 @@
+            ${EXTRA_VALA_OPTIONS}
+            -D USE_NATIVE_FILECHOOSER
+        )

I would prefer to set a define such as "GTK_3_20" rather than USE_NATIVE_FILECHOOSER since there will be other 3.20 features that we'd like to be able to include in the future.

::: src/client/dialogs/attachment-dialog.vala
@@ +41,1 @@
+        chooser.update_preview.connect(on_update_preview);

My reading of the FileChooserNative docs is that if we hook anything up to the update-preview signal like this, it means that the chooser will fall back to using a non-native chooser implementation. So when packaged for Flatpak, users won't be able to select attachments from outside the sandbox, which is a problem. The only solution to that would then be to not use previews when FileChooserNative is in use, which makes this class mostly redundant.

I would consider removing this class completely, and simply use either FileChooserNative or FileChooserDialog in its place. This means getting rid of previews altogether, but that's fine since the platform should really should be handling that anyway, not Geary.

Err, the first review comment referred to the addition of the USE_NATIVE_FILECHOOSER CMake option - not sure what it is missing from the comment.

Actually update-preview can be used with FileChooserNative, but not everywhere: on windows it will automatically fall backs to FileChooserDialog in place of IFileDialog, but for other platforms it's OK.
Having the preview is actually quite cool, hence the compilation option:
- we should use FileChooserNative only with Flatpak and/or on Windows for better system integration (and probably because IFileDialog has a preview but I did not check that).
- however on gtk builds, there is no interest in using FileChooserNative since this is actually a FileChooserDialog (without preview) - we don't gain much here I believe (we actually lose the color of the confirmation button). As Emmanuele Bassi stated "FileChooserNative" is a limited version of "FileChooserDialog", so in my opinion we should stuck with a FileChooserDialog as much as possible?

Anyway all my assumptions are based on the fact that image preview is cool; if we drop that then I agree with the rest of your arguments; that's up to you :).

Yep, I like the preview as well, but my understanding is that we'll have a similar problem under Flatpak as under Win32: Using FileChooserNative will invoke a DBus call to a portal, which is what displays the FileChooserDialog and passes the chosen file back to the sandbox via a shared file handle. Hence Geary won't have access to the portal's FileChooserDialog, and won't be able to display a preview. More details here https://blogs.gnome.org/mclasen/2016/07/08/portals-using-gtk-in-a-flatpak/

The question is then - will GTK+ in this case fall back to using FileChooserDialog under Flatpak as it does for Win32, or will it just ignore the preview? I'm not sure, but the former seems more sensible from an implementation perspective.

I guess we could find out by simply landing this as-is on master and waiting for the Flatpak nightly build to occur, then test it out, or maybe you could ask on the Flatpak list (xdg-apps)? Do you have a preference? Where did you hear ebassi mention this?

I am actually quite discovering flatpak and trying to build Geary yet so I could not check both theories; maybe landing it to master would be faster but otherwise I should be able to build it anytime soon.

No problem, I'd rather know what the complete story is before changing things.

Geary has a flatpak-builder config in https://git.gnome.org/browse/gnome-apps-nightly/ you can use, if that help. It currently takes a while to build since we need to also build WebKitGTK, but that will be a lot better once the WebKit2 port lands since we can use that from the Platform.

Actually I don't know why I did not reply to this question earlier, because it's clearly stated in the documentation:

(section Portail details of https://developer.gnome.org/gtk3/unstable/gtk3-GtkFileChooserNative.html):
In this situation, the following things are not supported and will be silently ignored:
    Extra widgets added with gtk_file_chooser_set_extra_widget().
    Use of custom previews by connecting to “update-preview”.
    Any GtkFileFilter added with a custom filter.



Anyway, I finally succeeded to build a flatpak geary, using the following (for reference :)).

git clone git://git.gnome.org/gnome-apps-nightly
cd gnome-apps-nightly/
flatpak install gnome org.gnome.Platform 3.22
flatpak install gnome org.gnome.Sdk 3.22
sed -i 's/runtime_version": "master"/runtime_version": "3.22"/' org.gnome.Geary.json
# I also modified org.gnome.Geary.json to use my git branch
flatpak-builder --repo=repo gearybuild org.gnome.Geary.json 3.22
# wait a few hours for webkitgtk to compile...
flatpak remote-add --no-gpg-verify repo file://$PWD/repo
flatpak install repo org.gnome.Geary
# there is a bug in the binary generated by flatpak, so I hacked by doing this:
mkdir ~/.var/app/org.gnome.Geary/config/geary 
flatpak run org.gnome.Geary

As a result, the attach button opens a dbus portal as expected (don't forget to install xdg-desktop-portal-gtk and xdg-desktop-portal!), and preview is silently ignored.
It was a week-long experience, but still interesting to see flatpak in action.

Also I forgot to mention that if xdg-desktop-portal is missing, filechoosernative should fallback automatically to filechooserdialog instead, but it is not yet available in gtk+ (see https://mail.gnome.org/archives/commits-list/2016-June/msg02349.html). 
So for now if you have no portal available, the attach button won't do anything.

Oh, good news! Okay, so I'm happy to keep the preview then. A compile-time option is still overkill however, so if you can drop that and address the other two points in comment 4 above, then we can get this committed.

Created attachment 336314 [details] [review]
patch proposal v3

Hopefully I addressed all concerns!

Hello, any news on this Michael (just bumping in case you missed it ;)).

Review of attachment 336314 [details] [review]:

Oh, thanks for the reminder, I had indeed missed it. Just one thing that wasn't addressed from v2:

::: CMakeLists.txt
@@ +86,3 @@
 
+pkg_check_modules(GTK3 QUIET gtk+-3.0>=3.14.0)
+

We already have this check in src/CMakeLists.txt - do we need it twice? If so, the desired minimum version should be set as a variable similar to TARGET_GLIB and used both here and there.

Created attachment 336833 [details] [review]
patch proposal v4

My bad, I thought I already fixed that issue but I actually did not commit it.

Review of attachment 336833 [details] [review]:

No probs. This has been pushed to master as 65fed2c. Thanks again!

The last thing to do here is to try dropping the RO/RW perms for host and home in the org.gnome.Geary.json builder conf in gnome-apps-nightly and see if anything breaks as a result.

In particular, the way Bug 712995 has been implemented may be problematic in terms of how inserted images are displayed in the composer and subsequently attached when sent. I'll test that out when I have a moment.

Created attachment 336857 [details] [review]
fix invalid gtk3 detection

Oops, pkg_check_modules does not use cmake proper syntax when exporting variables resulting in GTK_3_20 never being declared... please find the fix attached.

In the mean time I tested without read/write permissions and it works! (Though the native filechooser hangs when you try to type '/usr' in the path bar and become unresponsive - but this is not related to geary I believe).

Review of attachment 336857 [details] [review]:

Thanks for following that up. I was just starting to wonder why my Flatpak build's file choosers were't showing anything. :)

Pushed to master as a8930e5.

Oh, you know what else we both forgot? The Save/Save All attachments dialogs.

I was running a local Flatpak build, and Attach and Insert Image were both working fine (the latter needs this xdg-desktop-portal PR to work however: https://github.com/flatpak/xdg-desktop-portal/pull/69), however neither saving attachments or saving all attachments is working.

I think these were in an earlier version of the patch, can you see if you still have those changes lying around?

Created attachment 336966 [details] [review]
fix save/save all filechooser

You are right; it has been removed between version 2 and 3, when I tried to fix my white-spaces mess…

Review of attachment 336966 [details] [review]:

Cheers, I just pushed this as acb6b70.

I only noticed afterwards, but do you get the Save/Cancelled button text labels swapped? I'm on GTK+ 3.20 here and the Save button is on the left, but acts as a Cancel, and vice versa.

This is looking pretty solid under Flatpak now, so closing as fixed.