GNOME Bugzilla – Bug 700716
Support previewing of password protected documents
Last modified: 2014-02-25 10:14:18 UTC
Documents should be able to handle password protected content -- PDFs, ODTs, etc..
Created attachment 245388 [details] [review] mainToolbar: Don't forward key presses to the search bar on load-error
Created attachment 245389 [details] [review] pdf-loader: Simplify reference counting I hope I am not missing something in this one.
Created attachment 245390 [details] [review] Support password protected PDFs
Created attachment 245391 [details] Screenshot: trying to open a file named password-protected.pdf
Created attachment 245393 [details] [review] Support password protected PDFs Forgot to 'git add src/mainToolbar.js'.
Created attachment 245395 [details] [review] Support password protected PDFs This time I forgot to update the call to load from the print method.
Review of attachment 245389 [details] [review]: I probably didn't realize ev_job_scheduler_push_job() took a reference to the job object when writing that code. If you tested this to work, looks good to me.
Review of attachment 245395 [details] [review]: Thanks, looks great. Just a couple of minor comments below. ::: src/embed.js @@ +241,3 @@ + vgrid.add(entryGrid); + + row_spacing: 18, Why activates_default = FALSE if the button is can_default = TRUE? @@ +273,3 @@ + let file = new Gio.File.new_for_uri(doc.uri); + let msg = _("The document “%s” is locked and requires a password before it can be opened." + let entryGrid = new Gtk.Grid({ column_spacing: 12, Can you use doc.name instead? ::: src/lib/gd-pdf-loader.c @@ +1016,3 @@ + * @user_data: + * + * @passwd: (allow-none): The method doesn't return anything, so the Returns annotation should be removed.
Review of attachment 245388 [details] [review]: Looks good.
(In reply to comment #7) > Review of attachment 245389 [details] [review]: > > I probably didn't realize ev_job_scheduler_push_job() took a reference to the > job object when writing that code. If you tested this to work, looks good to > me. Yes, I tested it and it works for me.
(In reply to comment #8) > Review of attachment 245395 [details] [review]: Thanks for the review. > ::: src/embed.js > @@ +241,3 @@ > + vgrid.add(entryGrid); > + > + row_spacing: 18, > > Why activates_default = FALSE if the button is can_default = TRUE? I don't know. :-) It is a remnant of me experimenting with the code when keypresses were being forwarded to the searchbar and the activate default widget machinery wasn't working. Fixed. > @@ +273,3 @@ > + let file = new Gio.File.new_for_uri(doc.uri); > + let msg = _("The document “%s” is locked and requires a password > before it can be opened." > + let entryGrid = new Gtk.Grid({ column_spacing: 12, > > Can you use doc.name instead? Done. Reading src/documents.js, I see that there doc.name can be theoretically ''. Can it actually happen in practice? > ::: src/lib/gd-pdf-loader.c > @@ +1016,3 @@ > + * @user_data: > + * > + * @passwd: (allow-none): > > The method doesn't return anything, so the Returns annotation should be > removed. Fixed.
Created attachment 245442 [details] [review] Support previewing of password protected PDFs
Review of attachment 245442 [details] [review]: Looks good to me
(In reply to comment #11) > Done. Reading src/documents.js, I see that there doc.name can be theoretically > ''. Can it actually happen in practice? doc.name should already fall back to the filename, in case the value returned by Tracker is empty. It's also already used everywhere else, so I don't think this should be a problem in practice.
(In reply to comment #4) > Created an attachment (id=245391) [details] > Screenshot: trying to open a file named password-protected.pdf It looks a bit odd embedding the password entry in the view itself. The controls float in space. Here's some mockups for using dialogs instead: https://raw.github.com/gnome-design-team/gnome-mockups/master/documents/wireframes/password-protected-docs.png https://raw.github.com/gnome-design-team/gnome-mockups/master/documents/wireframes/password-protected-docs-batch.png
Debarshi, can we land this ? (and backport for f19, ideally)
Created attachment 248624 [details] [review] Support previewing of password protected PDFs
Review of attachment 248624 [details] [review]: Thanks Debarshi, this looks good with this minor style fix. ::: src/password.js @@ +79,3 @@ + mnemonic_widget: entry, + use_underline: true }); + label.get_style_context ().add_class('dim-label'); Extra space here.
Created attachment 248665 [details] [review] Support previewing of password protected PDFs
Reopening.
Review of attachment 248665 [details] [review]: This patch is incomplete. Among other things it breaks the fix for bug 701264 The issue is that PreviewToolbar listens to DocumentManager::load-{started, finished} to enable and disable various UI elements. However, due to this patch the toolbar never receives those signals because the toolbar gets created after those signals have been emitted. ::: src/embed.js @@ +387,3 @@ _onLoadFinished: function(manager, doc, docModel) { + Application.modeController.setWindowMode(WindowMode.WindowMode.PREVIEW); + Due to this hunk and the one above it, the mode is changed after load-{started, finished, etc.} have been emitted. Which means that PreviewToolbar did not get a chance to receive those signals.
Created attachment 270103 [details] [review] preview: The toolbar is created after load-{started, finished}
Created attachment 270104 [details] [review] embed: Handle the password dialog being deleted
Review of attachment 270103 [details] [review]: Looks good, but why do you need this._handleEvent at all?
Review of attachment 270104 [details] [review]: Sure
(In reply to comment #24) > Review of attachment 270103 [details] [review]: > > Looks good, but why do you need this._handleEvent at all? It was introduced in: commit 983f439c8348ca04eff6b911c5a05ca52e7bd872 Author: Debarshi Ray <debarshir@gnome.org> Date: Mon May 27 16:27:51 2013 +0200 mainToolbar: Don't forward key presses to the search bar on load-error https://bugzilla.gnome.org/show_bug.cgi?id=700716 The idea was to have a way of not forwarding key presses to the search bar.
Comment on attachment 270103 [details] [review] preview: The toolbar is created after load-{started, finished} OK I missed that. Looks good then.
Comment on attachment 270103 [details] [review] preview: The toolbar is created after load-{started, finished} Thanks for the review!