GNOME Bugzilla – Bug 746736
Let GtkApplication load the app menu automatically
Last modified: 2016-06-03 14:13:08 UTC
We need to change our resource base path to /org/gnome/Documents to match the default value computed by GIO.
Created attachment 300269 [details] [review] Let GtkApplication load the app menu automatically
Review of attachment 300269 [details] [review]: Doesn't that break with gnome-books?
Review of attachment 300269 [details] [review]: Also needs to be rebased to latest master I think. ::: src/application.js @@ +469,1 @@ cssProvider.load_from_file(file); This could use the load_from_resource() function instead.
Created attachment 323373 [details] [review] Let GtkApplication load the app menu automatically
Created attachment 326600 [details] [review] Let GtkApplication load the app menu automatically
Created attachment 326601 [details] [review] Let GtkApplication load the app menu automatically
Created attachment 326603 [details] [review] Let GtkApplication load the app menu automatically
Created attachment 326604 [details] [review] Let GtkApplication load the app menu automatically
Review of attachment 326604 [details] [review]: Looks good for me.
Review of attachment 326604 [details] [review]: Feel free to push to master.
Attachment 326604 [details] pushed as 0c0f72d - Let GtkApplication load the app menu automatically
This broke the menu loading in gnome-books.
commit 1a6c032641a7f76a3392c11b73a6ea7695213c47 Author: Bastien Nocera <hadess@hadess.net> Date: Tue May 31 19:26:17 2016 +0200 Revert "Let GtkApplication load the app menu automatically" This reverts commit 0c0f72d22d695a97f663c79d689fe6fb4c4b1cbe. This broke the app menu in gnome-books. See https://bugzilla.gnome.org/show_bug.cgi?id=746736#c12
How was menu loading broken by the patch exactly? I assume we need to load it from a different resource for the isBooks case?
(In reply to Cosimo Cecchi from comment #14) > How was menu loading broken by the patch exactly? It doesn't load the menu automatically at all, as the app assumes that the resource will be called org.gnome.Books.gresource and contain /org/gnome/Books/... But even if the resource is the right name, the path will be /org/gnome/Documents/ as we share the resource itself. > I assume we need to load > it from a different resource for the isBooks case? A different resource, and all the other /org/gnome/Documents/ paths would need to be changed as well. So to remove those 5 lines of code, you'd need to add about a dozen "if (isBooks)" checks in other sources. Maybe a per-app constant is a nicer way to fix this, I don't really mind. In any case both gnome-books and gnome-documents will need to be tested.
Created attachment 328989 [details] [review] Let GtkApplication load the app menu automatically
The attached patch fix both the app-menu loaded automatically and the shortcut window not displayed for Books.
Review of attachment 328989 [details] [review]: Looks good to me. Bastien, could you confirm that this also works for you?
(In reply to Cosimo Cecchi from comment #18) > Review of attachment 328989 [details] [review] [review]: > > Looks good to me. Bastien, could you confirm that this also works for you? Somebody touched the same code path in a commit, so the patch doesn't apply any more (Who could do that!)
Created attachment 329036 [details] [review] Let GtkApplication load the app menu automatically
Works for me (no errors, menu appears in both applications).
Attachment 329036 [details] pushed as 09a5e4f - Let GtkApplication load the app menu automatically