On Thu Nov 21, 2019 at 1:57 PM Greg Pomerantz wrote:
> RegisterFragment creates an instance of a Java class and attempts> to register it as a Fragment in the window's Context.> > diff --git a/app/app.go b/app/app.go> index 9a18ec4..71bc57b 100644> --- a/app/app.go> +++ b/app/app.go> diff --git a/app/app_android.go b/app/app_android.go> new file mode 100644> index 0000000..91fb4f1> --- /dev/null> +++ b/app/app_android.go> @@ -0,0 +1,29 @@> +// RegisterFragment constructs a Java instance of the specified class> +// and attempts to register it as a Fragment in the Context in which> +// the View was created.> +//> +// NOTE: This method must not be called from the Gio application's main> +// event loop, as that would block the View's UI thread and result in a> +// deadlock.> +func (w *Window) RegisterFragment(del string) error {> + if w.driver == nil {> + return errors.New("RegisterFragment: no window driver found")> + }> + d := w.driver.(window.AndroidDriver)
The access to w.driver needs to happen from the w.run goroutine. Change
RegisterFragment to pass the fragment name through a channel and send the error
back from another.
> diff --git a/app/internal/window/GioView.java b/app/internal/window/GioView.java> index 82bc36f..d601269 100644> --- a/app/internal/window/GioView.java> +++ b/app/internal/window/GioView.java> @@ -207,6 +222,59 @@ public class GioView extends SurfaceView implements Choreographer.FrameCallback> return onBack(nhandle);> }> > + public String registerFragment(String del) {> + final Class cls;> + final Fragment frag;> + try {> + cls = getContext().getClassLoader().loadClass(del);> + frag = (Fragment)cls.newInstance();
You should instantiate the class from the UI Thread. The user likely expects the
constructor to be able to access UI state.
> + }> + catch (ClassNotFoundException ignored) {
If it's all the same to you, I prefer to have the catch statement on the same line as the
ending }:
try {
} catch (...) {
}
> + return "RegisterFragment: fragment class not found";> + }> + catch (IllegalAccessException | InstantiationException | ExceptionInInitializerError ignored) {> + return "RegisterFragment: cannot instantiate delegate object";> + }> + catch (SecurityException ignored) {> + return "RegisterFragment: security exception";> + }> + catch (ClassCastException ignored) {> + return "RegisterFragment: provided class does not extend Fragment";
Collapse all these exception types to one error that includes the detail from exception.getMessage().
> + }> +> + final BlockingQueue<String> q = new LinkedBlockingQueue<String>();> +> + handler.post(new Runnable() {> + public void run() {> + final Context ctx = getContext();> + if (ctx == null) {> + q.add("RegisterFragment: GioView has null context");> + return;> + }> + final FragmentManager fm;> + try {> + final Method mth = ctx.getClass().getMethod("getFragmentManager");> + fm = (FragmentManager)mth.invoke(ctx);
Cast ctx to Activity and call getFragmentManager directly.
> + }> + catch (NoSuchMethodException | NullPointerException | InvocationTargetException | ClassCastException | IllegalAccessException ignored) {> + q.add("RegisterFragment: Cannot get Fragment manager from View Context");> + return;> + }> + q.add("");> + FragmentTransaction ft = fm.beginTransaction();> + ft.add(frag, del);
Call setRetained(true) on fragment, and note that in the documentation?
The user can call setRetained(true) themselves, but they may not realize it's
necessary to handle Activity re-creation.
> diff --git a/app/internal/window/handle.go b/app/internal/window/handle.go> deleted file mode 100644> index 5d7b1a1..0000000> diff --git a/app/internal/window/os_android.go b/app/internal/window/os_android.go> index 5cce88d..74d7482 100644> --- a/app/internal/window/os_android.go> +++ b/app/internal/window/os_android.go> @@ -54,6 +54,7 @@ type window struct {> mhideTextInput C.jmethodID> mpostFrameCallback C.jmethodID> mpostFrameCallbackOnMainThread C.jmethodID> + mRegisterFragment C.jmethodID> }> > var dataDirChan = make(chan string, 1)> @@ -119,6 +120,7 @@ func onCreateView(env *C.JNIEnv, class C.jclass, view C.jobject) C.jlong {> mhideTextInput: jniGetMethodID(env, class, "hideTextInput", "()V"),> mpostFrameCallback: jniGetMethodID(env, class, "postFrameCallback", "()V"),> mpostFrameCallbackOnMainThread: jniGetMethodID(env, class, "postFrameCallbackOnMainThread", "()V"),> + mRegisterFragment: jniGetMethodID(env, class, "registerFragment", "(Ljava/lang/String;)Ljava/lang/String;"),> }> wopts := <-mainWindow.out> w.callbacks = wopts.window> @@ -443,6 +445,30 @@ func (w *window) ShowTextInput(show bool) {> })> }> > +type AndroidDriver interface {> + RegisterFragment(string) error> +}
You only need this type in package app, right? If so, move it to package app
and unexport.
On 11/22/19 9:13 AM, Elias Naur wrote:
> On Thu Nov 21, 2019 at 1:57 PM Greg Pomerantz wrote:> The access to w.driver needs to happen from the w.run goroutine. Change> RegisterFragment to pass the fragment name through a channel and send the error> back from another.
Ok, are you looking for me to send this as a new kind of FragmentEvent
(via app.Window.in) or in a new channel? If we use Window.in, I'll
define a FragmentEvent struct that includes the class name (string) and
a chan error to return errors (or to be closed on success).
Fragment.getContext() should always return the associated Context
regardless of where the constructor is called, so I'm not sure it is
necessary to move this to the UI thread.
> You should instantiate the class from the UI Thread. The user likely expects the> constructor to be able to access UI state.
The way I am using it currently, I don't do anything work in the
constructor, because we are not yet attached to a Context and we can't
get access to class loaders, system services or anything else of value.
onAttach() is a better place for setup, as that will get the Context of
whatever the Fragment is attached to. It will also get called again if
the parent Activity is destroyed and re-created. This should all be
documented but I believe if all required setup is done there, there is
no need for setRetained(true), which is probably bad practice in general
anyway. There may be cases where Fragments have long-standing resources
that are time-consuming to set up where setRetained is appropriate but I
don't think it should be set in the general case.
> Call setRetained(true) on fragment, and note that in the documentation?> The user can call setRetained(true) themselves, but they may not realize it's> necessary to handle Activity re-creation.
I have confirmed that the Fragment will get re-attached when the
Activity is replaced (using your trick about removing the Manifest entry
about screen rotations), so I think we should leave this alone and
require the user to set it if they really need to.
>> +type AndroidDriver interface {>> + RegisterFragment(string) error>> +}> You only need this type in package app, right? If so, move it to package app> and unexport.
We need this so package app can call into internal/window, there needs
to be an exported function (or else we would need to export
window.window, which would require changes to the code for other
platforms). I did it this way so I would not have to extend the
internal/window/Driver interface and put empty functions in all of the
other platforms.
On Fri Nov 22, 2019 at 10:30 AM Gregory Pomerantz wrote:
> On 11/22/19 9:13 AM, Elias Naur wrote:> > On Thu Nov 21, 2019 at 1:57 PM Greg Pomerantz wrote:> > The access to w.driver needs to happen from the w.run goroutine. Change> > RegisterFragment to pass the fragment name through a channel and send the error> > back from another.> > > Ok, are you looking for me to send this as a new kind of FragmentEvent > (via app.Window.in) or in a new channel? If we use Window.in, I'll > define a FragmentEvent struct that includes the class name (string) and > a chan error to return errors (or to be closed on success).
SGTM, except that the event name will be unexported ("fragmentEvent")
> Fragment.getContext() should always return the associated Context > regardless of where the constructor is called, so I'm not sure it is > necessary to move this to the UI thread.>
You have to call all Android UI methods, including getContext, on the UI thread.
Perhaps it's enough to know that the UI thread is blocked while calling
getContext, but it doesn't cost us anything to be safe.
> > > You should instantiate the class from the UI Thread. The user likely expects the> > constructor to be able to access UI state.> > > The way I am using it currently, I don't do anything work in the > constructor, because we are not yet attached to a Context and we can't > get access to class loaders, system services or anything else of value.
Sure, but other users might do something interesting in the constructor.
For them it's a surprise the constructor is run from a non-UI thread.
> onAttach() is a better place for setup, as that will get the Context of > whatever the Fragment is attached to. It will also get called again if > the parent Activity is destroyed and re-created. This should all be > documented but I believe if all required setup is done there, there is > no need for setRetained(true), which is probably bad practice in general > anyway. There may be cases where Fragments have long-standing resources > that are time-consuming to set up where setRetained is appropriate but I > don't think it should be set in the general case.>
Ok, thank you for investigating. It's been a while I've done native
Android work.
> > > Call setRetained(true) on fragment, and note that in the documentation?> > The user can call setRetained(true) themselves, but they may not realize it's> > necessary to handle Activity re-creation.> > > I have confirmed that the Fragment will get re-attached when the > Activity is replaced (using your trick about removing the Manifest entry > about screen rotations), so I think we should leave this alone and > require the user to set it if they really need to.>
Great.
> > >> +type AndroidDriver interface {> >> + RegisterFragment(string) error> >> +}> > You only need this type in package app, right? If so, move it to package app> > and unexport.> > > We need this so package app can call into internal/window, there needs > to be an exported function (or else we would need to export > window.window, which would require changes to the code for other > platforms). I did it this way so I would not have to extend the > internal/window/Driver interface and put empty functions in all of the > other platforms.>
Understood, but the interface AndroidDriver itself is not needed in
internal/window, only the method, right? If so, the interface can be
moved to package app (but not the implementing method).
On 11/23/19 1:59 AM, Elias Naur wrote:
> BTW, please send further patches to the ~eliasnaur/gio-patches mailing> list, which I recently> added to separate discussion from patches.
FYI just sent this over to the patches list. A proof-of-concept
Bluetooth LE fragment that can scan and connect to devices is also running.