GNOME Bugzilla – Bug 781533
Disable the print button in the selection toolbar when printing isn't supported
Last modified: 2018-08-24 12:44:02 UTC
Now that we don't convert everything to PDFs, we might not be able to print everything that's not a collection. eg., we can't print EPUBs, ODFs and OOXMLs. Therefore it is not enough to only check for collections.
It bothers me that our 'feature testing' logic is getting duplicated. We have the canFoo methods in DocCommon to indicate whether a particular document supports a certain feature, and some of that is implicitly encoded in the list of GActions exported by the Preview sub-classes. I guess such 'feature tests' are hinged on the DocCommon sub-class, the Preview sub-class which in turn depends on the MIME type. I wonder if we can/should consolidate the logic in one place.
Created attachment 350129 [details] [review] Optionally load the document when determining if it can be printed
Created attachment 350130 [details] [review] documents: Check whether it is a collection in canPrint
Created attachment 350131 [details] [review] selections: Enable printing only for documents handled by EvView
Review of attachment 350129 [details] [review]: I'm not really sure I understand why this is needed. canPrint() already takes a docModel, so why do you need to load it again?
Review of attachment 350130 [details] [review]: The collection check is OK to add, but you may need to change the code if the previous patch is changed.
Review of attachment 350131 [details] [review]: Oh I see now why you need to load the document to determine whether it can be printed. In this case, since you only need to load the document from this code path, could you not load it here, and keep canPrint() synchronous?
Review of attachment 350131 [details] [review]: Err, setting right review status.
(In reply to Debarshi Ray from comment #1) > It bothers me that our 'feature testing' logic is getting duplicated. We > have the canFoo methods in DocCommon to indicate whether a particular > document supports a certain feature, and some of that is implicitly encoded > in the list of GActions exported by the Preview sub-classes. > > I guess such 'feature tests' are hinged on the DocCommon sub-class, the > Preview sub-class which in turn depends on the MIME type. I wonder if we > can/should consolidate the logic in one place. Yeah, it would be nice to consolidate the checks; however I think it's fine for the action state to be decoupled from the document state, and have the latter drive the former.
(In reply to Cosimo Cecchi from comment #5) > Review of attachment 350129 [details] [review] [review]: > > I'm not really sure I understand why this is needed. canPrint() already > takes a docModel, so why do you need to load it again? (In reply to Cosimo Cecchi from comment #7) > Review of attachment 350131 [details] [review] [review]: > > Oh I see now why you need to load the document to determine whether it can > be printed. > In this case, since you only need to load the document from this code path, > could you not load it here, and keep canPrint() synchronous? I wanted to abstract out the details of 'canPrint' because it might be useful when consolidating the logic. Except in some cases the docModel is already available, so letting the caller pass it is an easy way to avoid some extra I/O. I don't have any strong opinion about this. I am happy to move the 'load' outside 'canPrint' and keep it synchronous as you said.
(In reply to Cosimo Cecchi from comment #9) > (In reply to Debarshi Ray from comment #1) > > It bothers me that our 'feature testing' logic is getting duplicated. We > > have the canFoo methods in DocCommon to indicate whether a particular > > document supports a certain feature, and some of that is implicitly encoded > > in the list of GActions exported by the Preview sub-classes. > > > > I guess such 'feature tests' are hinged on the DocCommon sub-class, the > > Preview sub-class which in turn depends on the MIME type. I wonder if we > > can/should consolidate the logic in one place. > > Yeah, it would be nice to consolidate the checks; however I think it's fine > for the action state to be decoupled from the document state, and have the > latter drive the former. Yes, that's what I have been thinking - to somehow base the presence / sensitivity of the GActions on the DocCommon methods. I haven't completely thought it through to arrive at a concrete solution, yet.
Created attachment 353323 [details] [review] documents: Check whether it is a collection in canPrint These patches keep the canPrint method synchronous, and one has to load the document model separately.
Created attachment 353324 [details] [review] documents: Error out if an attempt was made to load a collection
Created attachment 353325 [details] [review] selections: Enable printing only for documents handled by EvView
Review of attachment 353323 [details] [review]: ++
Review of attachment 353324 [details] [review]: ++
Review of attachment 353325 [details] [review]: Looks good
There was some fallout from this: https://gitlab.gnome.org/GNOME/gnome-documents/issues/7