GNOME Bugzilla – Bug 686461
Use separate views for collections and documents in the overview
Last modified: 2015-02-19 19:28:31 UTC
If you create a folder in Google Drive/Docs eg "MyFolder" and then create a document "MyDoc" inside the folder gnome-documents will show the document twice. The the folder and document appear in the main gnome-documents window. If you then click on the folder the document appears inside the folder. This is misleading for users as the location of the document in Google Drive/docs is /MyFolder/MyDoc But Gnome-Documents is presenting the following: /MyDocs /MyFolder/MyDocs
This is not specific to Google. GNOME Documents shows both collections (Google folders are treated as collections) and a list of all documents in the main view. So, if you only had local content and created some collections, you would see both the collections and the documents inside them in the main view. I have seen some discussion on changing this so that things are not mixed up like this.
Changing the summary to reflect this.
I like this idea. As the reporter explains, it will make the relationship between collections and "Recent" much clearer. The latest mockups have separate views for Recent, Collections, Shared and Starred: https://raw.github.com/gnome-design-team/gnome-mockups/master/documents/documents-collections.png
Lets implement this for GNOME 3.14. I am going to attach a series of patches based on my understanding of how things need to change after implementing this for gnome-photos this cycle. Then we can iterate further.
This represents how search should behave in the current split view designs: https://raw.github.com/gnome-design-team/gnome-mockups/master/music/wire-search.png
Created attachment 272146 [details] [review] mainToolbar: Remove useless signal connections
Created attachment 272148 [details] [review] mainToolbar: Do not change the title to reflect the search parameters
Review of attachment 272146 [details] [review]: Looks good
Review of attachment 272148 [details] [review]: OK to commit after branching
Removing ui-review because we have a design in place now, which we have started to implement.
I was just using Documents to add noticed this.I think it would make documents much easier to use. +1 for this. Thanks of all the hard work
*** Bug 735881 has been marked as a duplicate of this bug. ***
Created attachment 287071 [details] [review] documents: Do the Signals.addSignalMethods dance Not sure how things were working without this because DocumentManager is a pure JS type.
Created attachment 287076 [details] [review] Don't clear DocumentManager on every TrackerController refresh This is similar to this gnome-photos commit: https://git.gnome.org/browse/gnome-photos/commit/?id=56f106413d38e66ffd8a542abb4741fa0b68caff The difference is that the change in gnome-photos was done after we had already implemented split-views, while this is the other way round. I think that gnome-photos' evolution into its current form is very similar to what we are trying to do with gnome-documents, so there is value in not reinventing the wheel and having some synergy.
Created attachment 287078 [details] [review] Merge CollectionManager into DocumentManager This is similar to this: https://git.gnome.org/browse/gnome-photos/commit/?id=34edc98b448c122d07ead5f7d2af098b59fc0890 You can read the rationale in bug 737071
Created attachment 287097 [details] [review] windowMode: Add goBack
Review of attachment 287071 [details] [review]: ::: src/documents.js @@ +1318,3 @@ } }); +Signals.addSignalMethods(DocumentManager.prototype); Are you sure this is needed at all? DocumentManager inherits from BaseManager which already does the signals dance.
Review of attachment 287097 [details] [review]: ::: src/edit.js @@ +86,3 @@ this._viewAction.connect('activate', Lang.bind(this, function() { + Application.modeController.goBack(); I am not really sure using goBack() here is correct; conceptually, the action is "show document preview", not "go back to the previous mode". On the other hand, I would have expected to see the new function used for the back button. ::: src/windowMode.js @@ +44,3 @@ + }, + + _canFullscreenPolicy: function(mode) { Could you rename this to simply _canFullscreen()? @@ +47,3 @@ + let retval = false; + + if (mode == WindowMode.PREVIEW Please merge these on a single line @@ +60,3 @@ + + let oldMode = this._history.pop(); + } You could merge this and the previous check with if (!oldMode || oldMode == WindowMode.NONE)
(In reply to comment #17) > Review of attachment 287071 [details] [review]: > > ::: src/documents.js > @@ +1318,3 @@ > } > }); > +Signals.addSignalMethods(DocumentManager.prototype); > > Are you sure this is needed at all? DocumentManager inherits from BaseManager > which already does the signals dance. Well, it has been working without it till now. All I know is that pure JS classes need to do that, and even though it inherits from BaseManager, it has a few signals of its own too which are not there in the parent. So, the answer is that I don't know. :)
(In reply to comment #18) > Review of attachment 287097 [details] [review]: > > ::: src/edit.js > @@ +86,3 @@ > this._viewAction.connect('activate', Lang.bind(this, > function() { > + Application.modeController.goBack(); > > I am not really sure using goBack() here is correct; conceptually, the action > is "show document preview", not "go back to the previous mode". True, but we can't just do another setWindowMode because that will add another entry in the stack, and then when you do goBack from the preview you will end up in EDIT instead of OVERVIEW. I went with goBack because it matched my mental model of going from OVERVIEW -> PREVIEW -> EDIT and then coming back. I am open to ideas, though. Anyway, I think we should atleast enforce the relation between PREVIEW and EDIT (and possibly other modes) in our state tracking. The thing is that we don't really need a stack to track the modes unless we have the multiple views. So, I could revert this one line and wait till we have some more code. > On the other hand, I would have expected to see the new function used for the > back button. I did not use it for the back button to avoid having code like this in the rest of the application: Application.documentManager.setActiveItem(null); Application.modeController.goBack(); Ideally the app should have one knob to turn and the state machine should take care of the details. This makes me wonder whether we should merge ModeController and DocumentManager. > ::: src/windowMode.js > @@ +44,3 @@ > + }, > + > + _canFullscreenPolicy: function(mode) { > > Could you rename this to simply _canFullscreen()? Problem is that there is already a variable called this._canFullscreen. I have a couple of patches to rearrange the fullscreen code to get rid of this variable and simplify things. I'll attach them separately, and if you want we can shuffle the patches around. > @@ +47,3 @@ > + let retval = false; > + > + if (mode == WindowMode.PREVIEW > > Please merge these on a single line Fixed. > @@ +60,3 @@ > + > + let oldMode = this._history.pop(); > + } > > You could merge this and the previous check with > > if (!oldMode || oldMode == WindowMode.NONE) Fixed.
Created attachment 288581 [details] [review] windowMode: Add goBack
Created attachment 288582 [details] [review] windowMode: Mark two methods as private
Created attachment 288583 [details] [review] windowMode: Clean up the fullscreen code
Created attachment 288760 [details] [review] search: Split OffsetController so that each view can have its own
Created attachment 288761 [details] [review] trackerController: Split it so that each view can have its own
Created attachment 288762 [details] [review] search: Add a helper to get all document SearchTypes
Created attachment 288763 [details] [review] mainToolbar: Create the selection menu only once during construction
Created attachment 288764 [details] [review] mainToolbar: Add a stack switcher for switching between the views
Created attachment 288767 [details] [review] embed, view: Move the no-results page into the ViewContainer
Adding as a 3.16 target.
Since Allan and the release team wants some movement regarding this, I will merge a few things from the branch wip/rishi/split-view that I consider 'safe'. Even then, it will be useful if people test and review them.
Created attachment 297247 [details] [review] Don't clear DocumentManager on every TrackerController refresh
Created attachment 297248 [details] [review] Merge CollectionManager into DocumentManager
Created attachment 297249 [details] [review] windowMode: Add goBack
Created attachment 297251 [details] [review] windowMode: Mark _setCanFullscreen as private
Created attachment 297252 [details] [review] windowMode: Clean up the fullscreen code
Created attachment 297267 [details] [review] windowMode: Support going back multiple steps
Created attachment 297268 [details] [review] mainWindow: Use a switch statement for consistency with similar code
Created attachment 297269 [details] [review] search: Split OffsetController so that each view can have its own
Created attachment 297270 [details] [review] trackerController: Split it so that each view can have its own
Review of attachment 287071 [details] [review]: I don't see a use for this.
Created attachment 297296 [details] [review] mainWindow: Use a switch statement for consistency with similar code
Created attachment 297297 [details] [review] search: Add a helper to get all document SearchTypes
Created attachment 297298 [details] [review] mainToolbar: Create the selection menu only once during construction
Created attachment 297303 [details] [review] mainToolbar: Add a stack switcher for switching between the views
Created attachment 297307 [details] [review] embed, view: Move the no-results page into the ViewContainer
Created attachment 297308 [details] [review] embed, view: Move the error page into the ViewContainer
Created attachment 297309 [details] [review] application: Let some actions be enabled across multiple window modes
Created attachment 297310 [details] [review] Rework the application accommodate multiple views
Created attachment 297311 [details] [review] application: Clean up OffsetController signals
Thanks for being patient and sorry for the delay.