GNOME Bugzilla – Bug 767083
books: Hide the "Present" menu item in Books
Last modified: 2016-06-03 14:12:56 UTC
.
Created attachment 328835 [details] [review] books: Hide the "Present" menu item in Books We're unlikely to make presentations with files from Books, so remove the menu item.
Created attachment 328836 [details] [review] books: Hide the "Present" menu item in Books We're unlikely to make presentations with files from Books, so remove the menu item.
Review of attachment 328836 [details] [review]: Few small comments on the patch - looks good in general. ::: src/application.js @@ +645,3 @@ + accels: ['F5'] }); + this._initActions(); + window_mode: WindowMode.WindowMode.PREVIEW, There should be no harm in calling this._initGettingStarted() before this._initActions() and this._initAppMenu(). So you could just have a block for the !isBooks case and call the other two methods later in a common code path. ::: src/preview.js @@ +148,2 @@ this._togglePresentation = Application.application.lookup_action('present-current'); + if (!Application.application.isBooks) { Is there any harm in connecting to the signal even if that particular detail will never be used? @@ +920,3 @@ + if (Application.application.isBooks) { + let section = builder.get_object('open-section'); + section.remove(3); Not a big fan of the hardcoding here; would it be possible to use a different menu definition for the isBooks case?
Created attachment 328956 [details] [review] books: Hide the "Present" menu item in Books We're unlikely to make presentations with files from Books, so remove the menu item.
(In reply to Cosimo Cecchi from comment #3) > Review of attachment 328836 [details] [review] [review]: > > Few small comments on the patch - looks good in general. > > ::: src/application.js > @@ +645,3 @@ > + accels: ['F5'] }); > + this._initActions(); > + window_mode: WindowMode.WindowMode.PREVIEW, > > There should be no harm in calling this._initGettingStarted() before > this._initActions() and this._initAppMenu(). > So you could just have a block for the !isBooks case and call the other two > methods later in a common code path. Done. > ::: src/preview.js > @@ +148,2 @@ > this._togglePresentation = > Application.application.lookup_action('present-current'); > + if (!Application.application.isBooks) { > > Is there any harm in connecting to the signal even if that particular detail > will never be used? You'll get a warning because the action doesn't exist (it's only added if !isBooks above in the patch). > @@ +920,3 @@ > + if (Application.application.isBooks) { > + let section = builder.get_object('open-section'); > + section.remove(3); > > Not a big fan of the hardcoding here; would it be possible to use a > different menu definition for the isBooks case? I remembered something I asked to implement in GMenu which solves this better. One-liner!
Review of attachment 328956 [details] [review]: Thank you, looks good now.
Attachment 328956 [details] pushed as 34d7c0a - books: Hide the "Present" menu item in Books
I get the following error: (org.gnome.Documents:12023): Gjs-WARNING **: JS ERROR: ReferenceError: presentCurrentId is not defined PreviewView<._init/<@resource:///org/gnome/Documents/js/preview.js:171 main@resource:///org/gnome/Documents/js/main.js:47 run@resource:///org/gnome/gjs/modules/package.js:192 @/home/abono/jhbuild/install/bin/gnome-documents:6
Created attachment 329037 [details] [review] preview: Increase scope of presentCurrentId
Review of attachment 329037 [details] [review]: Looks like the cleanest option. Thanks
Attachment 329037 [details] pushed as a399a86 - preview: Increase scope of presentCurrentId