GNOME Bugzilla – Bug 726450
Improve creating new collection
Last modified: 2015-08-25 11:05:08 UTC
In the new collection dialog, "Create new collection" needs to be pressed twice to start typing the name of the new collection. One should be able to start typing after pressing it once. When the "Create new collection" string is pressed once, it gets highlighted in a blue colour, which makes the string unreadable for me as the contrast is too low. Pressing the + in the label does not let one add a new collection (one needs to press the label specifically).
Yes, the current interaction isn't ideal for this. We have mockups for an improved collection creation UI: this would use a popover for adding a new collection. Details can be found here: https://wiki.gnome.org/Design/Whiteboards/Collections The collection creation popover should look like this: https://raw.github.com/gnome-design-team/gnome-mockups/master/notes/notes-add-to-notebook-dialog-new-notebook.png
Those designs look really good, looking forward to seeing them implemented!
(In reply to Allan Day from comment #1) The current URL is: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/collections/collections-wires-documents.png
I've moved things around a bit. The designs can now be found here: https://wiki.gnome.org/Design/OS/Collections Mockups are here: https://github.com/gnome-design-team/gnome-mockups/tree/master/collections
Created attachment 303279 [details] [review] selections: Mimic the mockup's look without any interactions
Review of attachment 303279 [details] [review]: Looks like a start, Alessandro. Some minor comments. ::: src/selections.js @@ +638,3 @@ + hexpand: true }); + let addBox = new Gtk.Box({ margin: 5 } ); + addBox.add(new Gtk.Entry({ hexpand: true, placeholder_text: _("New Collection...") })); Use a Unicode ellipsis (…). See https://wiki.gnome.org/Design/OS/Typography @@ +641,3 @@ + addBox.add(new Gtk.Button({ label: _("Add") })); + addBox.get_style_context().add_class("linked"); + this.widget.add(addBox); Maybe move the 'addBox' related bits above the 'this._sw' bits? @@ +647,3 @@ + this._view.set_header_func(Lang.bind(this, + function(row, before) { + if(before == null) { Missing space before opening parenthesis. Please use 'if (!before)'. That is what is most commonly used in the code. @@ +652,3 @@ + } + let current = row.get_header(); + if(current == null) { Ditto. @@ +658,3 @@ + } + })); + for(let i = 0; i < 20; i++) { Ditto.
Created attachment 304247 [details] [review] selections: new collections dialog Now collections are showed. There is a segmentation fault, steps to reproduce it: open the collections dialog, close it, then do something else (scrolling or opening a document)
Created attachment 304259 [details] backtrace
Created attachment 304346 [details] [review] selections: new collections dialog Now collections are showed. There is a segmentation fault, steps to reproduce it: open the collections dialog, close it, then do something else (scrolling or opening a document)
Review of attachment 304247 [details] [review]: Some comments: ::: src/selections.js @@ +329,2 @@ _init: function() { + this.model = new Gio.ListStore({ item_type: DocumentsCollection.$gtype }); Could you please bump the GLib requirement in configure.ac for this? @@ +689,3 @@ + let isInconsistent = (collection.state & OrganizeCollectionState.INCONSISTENT); + let listBoxRow = new Gtk.ListBoxRow({ selectable: false }); + let box = new Gtk.Box({ border_width: 5 }); GtkGrid is considered more modern than GtkBox, so it would be better if you can use it. @@ +699,3 @@ + box.pack_end(menuButton, false, false, 0); + listBoxRow.add(box); + return listBoxRow; You should directly return the widget that is supposed to go inside the GtkListBoxRow (ie. the box in this case).
Created attachment 304363 [details] [review] selections: new collections dialog
Created attachment 304483 [details] [review] selections: new collections dialog
(In reply to Debarshi Ray from comment #10) > Review of attachment 304247 [details] [review] [review]: > @@ +699,3 @@ > + box.pack_end(menuButton, false, false, 0); > + listBoxRow.add(box); > + return listBoxRow; > > You should directly return the widget that is supposed to go inside the > GtkListBoxRow (ie. the box in this case). Ignore this. As you pointed out on IRC, it can handle GtkListBoxRows just fine. Also, the crash you were seeing was fixed in bug 750286
Created attachment 305354 [details] [review] selections: new collections dialog
Comment on attachment 305354 [details] [review] selections: new collections dialog >From b3fb84db9fbb52f4e8eb8d6110e2328b7de0d9f8 Mon Sep 17 00:00:00 2001 >From: Alessandro Bono <shadow@openaliasbox.org> >Date: Tue, 16 Jun 2015 02:19:58 +0200 >Subject: [PATCH] selections: new collections dialog > >https://bugzilla.gnome.org/show_bug.cgi?id=726450 >--- > configure.ac | 4 +- > src/application.js | 9 +- > src/gnome-documents.gresource.xml | 1 - > src/resources/collections-placeholder.png | Bin 686 -> 0 bytes > src/selections.js | 471 +++++++++++++----------------- > 5 files changed, 210 insertions(+), 275 deletions(-) > delete mode 100644 src/resources/collections-placeholder.png > >diff --git a/configure.ac b/configure.ac >index ede904c..660b3a6 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -51,8 +51,8 @@ AC_SUBST(LIBM) > > EVINCE_MIN_VERSION=3.13.3 > WEBKITGTK_MIN_VERSION=2.6.0 >-GLIB_MIN_VERSION=2.39.3 >-GTK_MIN_VERSION=3.15.5 >+GLIB_MIN_VERSION=2.44.0 >+GTK_MIN_VERSION=3.16.0 > GOBJECT_INTROSPECTION_MIN_VERSION=1.31.6 > GDATA_MIN_VERSION=0.13.3 > GOA_MIN_VERSION=3.2.0 >diff --git a/src/application.js b/src/application.js >index cd42545..ea81530 100644 >--- a/src/application.js >+++ b/src/application.js >@@ -289,7 +289,8 @@ const Application = new Lang.Class({ > action = Gio.SimpleAction.new_stateful(actionEntry.name, > parameterType, actionEntry.state); > else >- action = new Gio.SimpleAction({ name: actionEntry.name }); >+ action = new Gio.SimpleAction({ name: actionEntry.name, >+ parameter_type: parameterType }); > > if (actionEntry.create_hook) > actionEntry.create_hook.apply(this, [action]); >@@ -635,7 +636,11 @@ const Application = new Lang.Class({ > state: GLib.Variant.new('s', Search.SearchMatchStock.ALL), > window_modes: [WindowMode.WindowMode.COLLECTIONS, > WindowMode.WindowMode.DOCUMENTS, >- WindowMode.WindowMode.SEARCH] } >+ WindowMode.WindowMode.SEARCH] }, >+ { name: 'delete-collection', >+ parameter_type: 's' }, >+ { name: 'rename-collection', >+ parameter_type: 's' } > ]; > > this._initActions(); >diff --git a/src/gnome-documents.gresource.xml b/src/gnome-documents.gresource.xml >index 982d1e1..225a73d 100644 >--- a/src/gnome-documents.gresource.xml >+++ b/src/gnome-documents.gresource.xml >@@ -7,7 +7,6 @@ > <file alias="preview-menu.ui" preprocess="xml-stripblanks">resources/preview-menu.ui</file> > <file alias="selection-menu.ui" preprocess="xml-stripblanks">resources/selection-menu.ui</file> > <file alias="preview-context-menu.ui" preprocess="xml-stripblanks">resources/preview-context-menu.ui</file> >- <file alias="collections-placeholder.png" preprocess="to-pixdata">resources/collections-placeholder.png</file> > <file alias="thumbnail-frame.png" preprocess="to-pixdata">resources/thumbnail-frame.png</file> > </gresource> > </gresources> >diff --git a/src/resources/collections-placeholder.png b/src/resources/collections-placeholder.png >deleted file mode 100644 >index 101296577b1d81c84656e149547e073a88c4f735..0000000000000000000000000000000000000000 >GIT binary patch >literal 0 >HcmV?d00001 > >literal 686 >zcmV;f0#W^mP)<h;3K|Lk000e1NJLTq001xm001xu1^@s6R|5Hm00004b3#c}2nYxW >zd<bNS00009a7bBm000fw000fw0YWI7cmMzZ8FWQhbW?9;ba!ELWdL_~cP?peYja~^ >zaAhuUa%Y?FJQ@H10xL;GK~!jg?V2%f8Zj7#-_OGT05PMiRnsY*O;PU`G+U-@>{<8) >zELB%BWnv>^7p@}bIoXK1qRxnaVClXNwW+xh0^y49ke|tXHun44KI^RSh?(J&rfEw` >zdBw~ZMD!8B8NxAV=0_rWGRBOOBv}+2kZ(Y1{e_u_0Gh{Cxg$#=>M5n}{h)9Kt@V(Z >z$Ec=w0BADvSZh7>gT%~k=EtFw?F$iIx|N{GWYUsS{>1vsF97t3=s_v<D}+b@&|0^d >z`4fNvfR<OaG{(G3l4Q}4QeL4T-Wc<K(*tvC?Bw00Y5HAC`Ha6NnpXEW!ORzh|MjaN >z-pA_c`!T1xVUNlcMD*aw)9LiH5aKI<^L+>HbUNh9S(fp(wl6Tt^ZcgU?f!7O{1emZ >z7ATza{+@rU_>wrc;{#aNWjO=9WAvp?N*1**ua5tkfyxsNgeDpYO*9aiXdpDvKxk@j >zps}see<ePIvijLX?N^}o2DWaYH{PTGlyW2cr=LyKeg&d|&_n~Fi3UOw4TL5d2+heG >zm>osM0pkk*b3dT9Zo6`x=Qq1_@}HR5@d2#svdjUXVe_!79D&VA-2s42E_^stNf|dX >ztIPlW2mpeJo(gRTX_~fBA*`N(A9K2eF=hl{>D9HQl+Rg~U2j;nqoTELXIXY#NbmuF >zvXmp>1oFfJ0F+YqM06Wg)@~5dt^c{f>KIi@^@!*a>)WYHEbYwq{2-mw*D7`7H^Q6k >UbSdx#82|tP07*qoM6N<$g4Y;8Gynhq > >diff --git a/src/selections.js b/src/selections.js >index 812eb3c..dd7b8d0 100644 >--- a/src/selections.js >+++ b/src/selections.js >@@ -22,6 +22,7 @@ > const EvView = imports.gi.EvinceView; > const Gd = imports.gi.Gd; > const Gdk = imports.gi.Gdk; >+const Gio = imports.gi.Gio; > const GLib = imports.gi.GLib; > const GObject = imports.gi.GObject; > const Gtk = imports.gi.Gtk; >@@ -41,9 +42,6 @@ const Utils = imports.utils; > const Lang = imports.lang; > const Signals = imports.signals; > >-const _COLLECTION_PLACEHOLDER_ID = 'collection-placeholder'; >-const _SEPARATOR_PLACEHOLDER_ID = 'separator-placeholder'; >- > // fetch all the collections a given item is part of > const FetchCollectionsJob = new Lang.Class({ > Name: 'FetchCollectionsJob', >@@ -310,81 +308,51 @@ const CreateCollectionJob = new Lang.Class({ > } > }); > >-const OrganizeModelColumns = { >- ID: 0, >- NAME: 1, >- STATE: 2 >-}; >+const DocumentsCollection = new Lang.Class({ >+ Name: 'DocumentsCollection', >+ Extends: GObject.Object, >+ >+ _init: function(id, name, mtime, state) { >+ this.id = id; >+ this.name = name; >+ this.state = state; >+ this.mtime = mtime; >+ this.parent(); >+ } >+}); > >-const OrganizeCollectionModel = new Lang.Class({ >- Name: 'OrganizeCollectionModel', >+const CollectionListModel = new Lang.Class({ >+ Name: 'CollectionListModel', >+ Extends: Gio.ListStore, > > _init: function() { >- this.model = Gtk.ListStore.new( >- [ GObject.TYPE_STRING, >- GObject.TYPE_STRING, >- GObject.TYPE_INT ]); >+ this.parent({ item_type: DocumentsCollection }); > > this._collAddedId = Application.documentManager.connect('item-added', > Lang.bind(this, this._onCollectionAdded)); > this._collRemovedId = Application.documentManager.connect('item-removed', > Lang.bind(this, this._onCollectionRemoved)); > >- let iter; >- >- // add the placeholder >- iter = this.model.append(); >- this.model.set(iter, >- [ 0, 1, 2 ], >- [ _COLLECTION_PLACEHOLDER_ID, '', OrganizeCollectionState.ACTIVE ]); >- >- // add the separator >- iter = this.model.append(); >- this.model.set(iter, >- [ 0, 1, 2 ], >- [ _SEPARATOR_PLACEHOLDER_ID, '', OrganizeCollectionState.ACTIVE ]); >- > // populate the model > let job = new FetchCollectionStateForSelectionJob(); > job.run(Lang.bind(this, this._onFetchCollectionStateForSelection)); > }, > >- _findCollectionIter: function(item) { >- let collPath = null; >- >- this.model.foreach(Lang.bind(this, >- function(model, path, iter) { >- let id = model.get_value(iter, OrganizeModelColumns.ID); >- >- if (item.id == id) { >- collPath = path.copy(); >- return true; >- } >- >- return false; >- })); >- >- if (collPath) >- return this.model.get_iter(collPath)[1]; >- >- return null; >- }, >- > _onFetchCollectionStateForSelection: function(collectionState) { >+ this.remove_all(); > for (let idx in collectionState) { > let item = Application.documentManager.getItemById(idx); > > if ((collectionState[item.id] & OrganizeCollectionState.HIDDEN) != 0) > continue; > >- let iter = this._findCollectionIter(item); >- >- if (!iter) >- iter = this.model.append(); >- >- this.model.set(iter, >- [ 0, 1, 2 ], >- [ item.id, item.name, collectionState[item.id] ]); >+ this.insert_sorted(new DocumentsCollection(item.id, item.name, item.mtime, collectionState[item.id]), Lang.bind(this, >+ function(docColl0, docColl1) { >+ // this doesn't work, arguments.length is 0 >+ //log(arguments.length); >+ //return docColl0.mtime - docColl1.mtime; >+ return -1; >+ })); > } > }, > >@@ -394,14 +362,30 @@ const OrganizeCollectionModel = new Lang.Class({ > }, > > _onCollectionAdded: function(manager, itemAdded) { >- this._refreshState(); >+ let newCollection = new DocumentsCollection(itemAdded.id, itemAdded.name, itemAdded.mtime, OrganizeCollectionState.ACTIVE); >+ this.insert_sorted(newCollection, Lang.bind(this, >+ function(docColl0, docColl1) { >+ //return docColl0.mtime - docColl1.mtime; >+ return -1; >+ })); > }, > > _onCollectionRemoved: function(manager, itemRemoved) { >- let iter = this._findCollectionIter(itemRemoved); >+ this._refreshState(); >+ }, >+ >+ checkIsValidName: function(name) { >+ if (!name || name == '') >+ return false; >+ >+ // we check if the name already exits: see bug 735863 >+ let item = null; >+ for(let i = 0; item = this.get_item(i); i++) { >+ if(item.name == name) >+ return false; >+ } > >- if (iter) >- this.model.remove(iter); >+ return true; > }, > > refreshCollectionState: function() { >@@ -421,247 +405,194 @@ const OrganizeCollectionModel = new Lang.Class({ > } > }); > >-const OrganizeCollectionView = new Lang.Class({ >- Name: 'OrganizeCollectionView', >- >- _init: function() { >- this._choiceConfirmed = false; >- >- this.widget = new Gtk.Overlay(); >- >- this._sw = new Gtk.ScrolledWindow({ shadow_type: Gtk.ShadowType.IN, >- margin_start: 5, >- margin_end: 5, >- margin_bottom: 3 }); >- this.widget.add(this._sw); >- >- this._model = new OrganizeCollectionModel(); >- this._view = new Gtk.TreeView({ headers_visible: false, >- vexpand: true, >- hexpand: true }); >- this._view.set_model(this._model.model); >- this._view.set_row_separator_func(Lang.bind(this, >- function(model, iter) { >- let id = model.get_value(iter, OrganizeModelColumns.ID); >- return (id == _SEPARATOR_PLACEHOLDER_ID); >- })); >- this._sw.add(this._view); >+const CollectionRow = new Lang.Class({ >+ Name: "CollectionRow", >+ Extends: Gtk.ListBoxRow, > >- this._msgGrid = new Gtk.Grid({ orientation: Gtk.Orientation.VERTICAL, >- row_spacing: 12, >- halign: Gtk.Align.CENTER, >- margin_top: 64 }); >- this.widget.add_overlay(this._msgGrid); >- >- this._icon = new Gtk.Image({ resource: '/org/gnome/documents/collections-placeholder.png' }); >- this._msgGrid.add(this._icon); >- >- this._label = new Gtk.Label({ >- justify: Gtk.Justification.CENTER, >- label: _("You don't have any collections yet. Enter a new collection name above."), >- max_width_chars: 32, >- wrap: true }); >- this._label.get_style_context().add_class('dim-label'); >- this._msgGrid.add(this._label); >- >- // show the overlay only if there aren't any collections in the model >- this._msgGrid.visible = (this._model.model.iter_n_children(null) < 2); >- this._model.model.connect('row-inserted', Lang.bind(this, >- function() { >- this._msgGrid.hide(); >- })); >+ _init: function(collection) { >+ this.parent(); >+ this._collection = collection; > >- // force the editable row to be unselected >- this.selection = this._view.get_selection(); >- let selectionChangedId = this.selection.connect('changed', Lang.bind(this, >- function() { >- this.selection.unselect_all(); >- if (selectionChangedId != 0) { >- this.selection.disconnect(selectionChangedId); >- selectionChangedId = 0; >- } >+ let actionRename = Application.application.lookup_action('rename-collection'); >+ actionRename.connect('activate', Lang.bind(this, >+ function(collId) { >+ this._onRenameMode(collId); > })); > >- this._view.connect('destroy', Lang.bind(this, >- function() { >- this._model.destroy(); >+ let actionDelete = Application.application.lookup_action('delete-collection'); >+ actionDelete.connect('activate', Lang.bind(this, >+ function(collId) { >+ this._onDeleteMode(collId); > })); > >- this._viewCol = new Gtk.TreeViewColumn(); >- this._view.append_column(this._viewCol); >- >- // checkbox >- this._rendererCheck = new Gtk.CellRendererToggle(); >- this._viewCol.pack_start(this._rendererCheck, false); >- this._viewCol.set_cell_data_func(this._rendererCheck, >- Lang.bind(this, this._checkCellFunc)); >- this._rendererCheck.connect('toggled', Lang.bind(this, this._onCheckToggled)); >- >- // icon >- this._rendererIcon = new Gtk.CellRendererPixbuf(); >- this._viewCol.pack_start(this._rendererIcon, false); >- this._viewCol.set_cell_data_func(this._rendererIcon, >- Lang.bind(this, this._iconCellFunc)); >- >- // item name >- this._rendererText = new Gtk.CellRendererText(); >- this._viewCol.pack_start(this._rendererText, true); >- this._viewCol.set_cell_data_func(this._rendererText, >- Lang.bind(this, this._textCellFunc)); >- >- this._rendererDetail = new Gd.StyledTextRenderer({ xpad: 16 }); >- this._rendererDetail.add_class('dim-label'); >- this._viewCol.pack_start(this._rendererDetail, false); >- this._viewCol.set_cell_data_func(this._rendererDetail, >- Lang.bind(this, this._detailCellFunc)); >- >- this._rendererText.connect('edited', Lang.bind(this, this._onTextEdited)); >- this._rendererText.connect('editing-canceled', Lang.bind(this, this._onTextEditCanceled)); >- >- this._view.show(); >+ this._onDefaultMode(); > }, > >- _onCheckToggled: function(renderer, pathStr) { >- let path = Gtk.TreePath.new_from_string(pathStr); >- let iter = this._model.model.get_iter(path)[1]; >+ _onDefaultMode: function() { >+ let isActive = (this._collection.state & OrganizeCollectionState.ACTIVE); >+ let isInconsistent = (this._collection.state & OrganizeCollectionState.INCONSISTENT); > >- let collUrn = this._model.model.get_value(iter, OrganizeModelColumns.ID); >- let state = this._rendererCheck.get_active(); >- >- let job = new SetCollectionForSelectionJob(collUrn, !state); >- job.run(Lang.bind(this, >+ let grid = new Gtk.Grid({ border_width: 5 }); >+ let checkButton = new Gtk.CheckButton({ label: this._collection.name, >+ expand: true, >+ active: isActive, >+ inconsistent: isInconsistent }); >+ checkButton.connect('toggled', Lang.bind(this, > function() { >- this._model.refreshCollectionState(); >- })); >- }, >+ let collId = this._collection.id; >+ let state = checkButton.get_active(); > >- _onTextEditedReal: function(cell, newText) { >- //cell.editable = false; >- >- if (!newText || newText == '') { >- // don't insert collections with empty names >- return; >- } >- >- // update the new name immediately >- let iter = this._model.model.append(); >- this._model.model.set_value(iter, OrganizeModelColumns.NAME, newText); >- >- // force the editable row to be unselected >- this.selection.unselect_all(); >- >- // actually create the new collection >- let job = new CreateCollectionJob(newText); >- job.run(Lang.bind(this, >- function(createdUrn) { >- if (!createdUrn) >- return; >- >- this._model.model.set_value(iter, OrganizeModelColumns.ID, createdUrn); >- >- let job = new SetCollectionForSelectionJob(createdUrn, true); >- job.run(null); >+ let job = new SetCollectionForSelectionJob(collId, state); >+ job.run(); > })); >- }, >- >- _onTextEdited: function(cell, pathStr, newText) { >- this._onTextEditedReal(cell, newText); >- }, > >- _onTextEditCanceled: function(cell) { >- if (this._choiceConfirmed) { >- this._choiceConfirmed = false; >+ let menu = new Gio.Menu(); >+ menu.insert(0, _("Renameâ¦"), 'app.rename-collection(\'' + this._collection.id + '\')'); >+ menu.insert(1, _("Delete"), 'app.delete-collection(\'' + this._collection.id + '\')'); > >- let entry = this._viewCol.cell_area.get_edit_widget(); >- if (entry) >- this._onTextEditedReal(cell, entry.get_text()); >- } >- }, >- >- _checkCellFunc: function(col, cell, model, iter) { >- let state = model.get_value(iter, OrganizeModelColumns.STATE); >- let id = model.get_value(iter, OrganizeModelColumns.ID); >+ let menuButton = new Gtk.MenuButton({ image: new Gtk.Image({ icon_name: 'open-menu-symbolic' }), >+ menu_model: menu }); > >- cell.active = (state & OrganizeCollectionState.ACTIVE); >- cell.inconsistent = (state & OrganizeCollectionState.INCONSISTENT); >- cell.visible = (id != _COLLECTION_PLACEHOLDER_ID); >+ grid.attach_next_to(checkButton, null, Gtk.PositionType.LEFT, 1, 1); >+ grid.attach_next_to(menuButton, null, Gtk.PositionType.RIGHT, 1, 1); >+ this._changeChild(grid); > }, > >- _iconCellFunc: function(col, cell, model, iter) { >- let id = model.get_value(iter, OrganizeModelColumns.ID); >+ _onDeleteMode: function(collId) { >+ if(this._collection.id != collId) >+ return; > >- cell.icon_name = "list-add"; >- cell.visible = (id == _COLLECTION_PLACEHOLDER_ID); >+ let grid = new Gtk.Grid({ border_width: 5 }); >+ let deleteLabel = new Gtk.Label({ label: '"' + this._collection.name + '" removed', expand: true }); >+ let undoButton = new Gtk.Button({ label: _("Undo") }); >+ grid.attach_next_to(deleteLabel, null, Gtk.PositionType.LEFT, 1, 1); >+ grid.attach_next_to(undoButton, null, Gtk.PositionType.RIGHT, 1, 1); >+ this._changeChild(grid); > }, > >- _textCellFunc: function(col, cell, model, iter) { >- let id = model.get_value(iter, OrganizeModelColumns.ID); >- let name = model.get_value(iter, OrganizeModelColumns.NAME); >+ _onRenameMode: function(collId) { >+ if(this._collection.id != collId) >+ return; > >- if (id == _COLLECTION_PLACEHOLDER_ID) { >- cell.editable = true; >- cell.text = ''; >- cell.placeholder_text = _("Create new collection"); >- } else { >- cell.editable = false; >- cell.text = name; >- } >+ let grid = new Gtk.Grid({ border_width: 5 }); >+ let renameEntry = new Gtk.Entry({ expand: true, text: this._collection.name, secondary_icon_name: 'edit-clear-symbolic'}); >+ renameEntry.connect('icon-press', Lang.bind(this, >+ function(renameEntry, iconPos) { >+ if(iconPos == Gtk.EntryIconPosition.SECONDARY) { >+ renameEntry.set_text(""); >+ renameEntry.grab_focus(); >+ } >+ })); >+ renameEntry.connect('changed', Lang.bind(this, >+ function() { >+ if(renameEntry.get_text() != "") >+ renameEntry.set_icon_from_icon_name(Gtk.EntryIconPosition.SECONDARY, 'edit-clear-symbolic'); >+ else >+ renameEntry.set_icon_from_icon_name(Gtk.EntryIconPosition.SECONDARY, null); >+ })); >+ renameEntry.grab_focus(); >+ grid.attach(renameEntry, 0, 0, 1, 1); >+ this._changeChild(grid); > }, > >- _detailCellFunc: function(col, cell, model, iter) { >- let id = model.get_value(iter, OrganizeModelColumns.ID); >- let item = Application.documentManager.getItemById(id); >+ _changeChild: function(newChild) { >+ let oldChild = this.get_child(); >+ if(oldChild) >+ this.remove(oldChild); > >- if (item && item.identifier.indexOf(Query.LOCAL_COLLECTIONS_IDENTIFIER) == -1) { >- cell.text = Application.sourceManager.getItemById(item.resourceUrn).name; >- cell.visible = true; >- } else { >- cell.text = ''; >- cell.visible = false; >- } >- }, >+ newChild.show_all(); >+ this.add(newChild); >+ } >+}); > >- confirmedChoice: function() { >- this._choiceConfirmed = true; >+const CollectionListView = new Lang.Class({ >+ Name: 'CollectionListView', >+ Extends: Gtk.ListBox, >+ >+ _init: function(model) { >+ this.parent({ vexpand: false, selection_mode: Gtk.SelectionMode.NONE }); >+ this._model = model; >+ this.set_header_func(Lang.bind(this, >+ function(row, before) { >+ if (!before) { >+ row.set_header(null); >+ return; >+ } >+ let current = row.get_header(); >+ if (!current) { >+ current = new Gtk.Separator({ orientation: Gtk.Orientation.HORIZONTAL }); >+ row.set_header(current); >+ } >+ })); >+ this.bind_model(this._model, Lang.bind(this, >+ function(collection) { >+ return new CollectionRow(collection); >+ })); > } > }); > > const OrganizeCollectionDialog = new Lang.Class({ > Name: 'OrganizeCollectionDialog', >+ Extends: Gtk.Dialog, > > _init: function(toplevel) { >- this.widget = new Gtk.Dialog({ transient_for: toplevel, >- modal: true, >- destroy_with_parent: true, >- use_header_bar: true, >- default_width: 400, >- default_height: 300, >+ this.parent({ transient_for: toplevel, >+ modal: true, >+ destroy_with_parent: true, >+ use_header_bar: true, >+ default_width: 350, >+ default_height: 350, > // Translators: "Collections" refers to documents in this context >- title: C_("Dialog Title", "Collections") }); >- >- let closeButton = this.widget.add_button('gtk-close', Gtk.ResponseType.CLOSE); >- this.widget.set_default_response(Gtk.ResponseType.CLOSE); >- >- let contentArea = this.widget.get_content_area(); >- let collView = new OrganizeCollectionView(); >- contentArea.add(collView.widget); >- >- // HACK: >- // - We want clicking on "Close" to add the typed-in collection if we're >- // editing. >- // - Unfortunately, since we focus out of the editable entry in order to >- // click the button, we'll get an editing-canceled signal on the renderer >- // from GTK. As this handler will run before focus-out, we here signal the >- // view to ignore the next editing-canceled signal and add the collection in >- // that case instead. >- // >- closeButton.connect('button-press-event', Lang.bind(this, >+ title: C_("Dialog Title", "Collections") }); >+ >+ let content = this.get_content_area(); >+ let addBox = new Gtk.Box({ margin: 5 }); >+ this._model = new CollectionListModel(); >+ this._view = new CollectionListView(this._model); >+ this.connect('destroy', Lang.bind(this, > function() { >- collView.confirmedChoice(); >- return false; >+ this._model.destroy(); > })); > >- this.widget.show_all(); >+ let addEntry = new Gtk.Entry({ hexpand: true, placeholder_text: _("New Collectionâ¦") }); >+ let addButton = new Gtk.Button({ label: _("Add"), sensitive: false }); >+ addEntry.connect('changed', Lang.bind(this, >+ function() { >+ let isValidName = this._model.checkIsValidName(addEntry.get_text()); >+ addButton.set_sensitive(isValidName); >+ })); >+ addButton.connect('clicked', Lang.bind(this, >+ function() { >+ let newText = addEntry.get_text(); >+ let job = new CreateCollectionJob(newText); >+ job.run(Lang.bind(this, >+ function(createdUrn) { >+ if (!createdUrn) >+ return; >+ >+ addEntry.set_text(''); >+ let job = new SetCollectionForSelectionJob(createdUrn, true); >+ job.run(null); >+ })); >+ })); >+ addBox.add(addEntry); >+ addBox.add(addButton); >+ addBox.get_style_context().add_class("linked"); >+ content.add(addBox); >+ >+ this._sw = new Gtk.ScrolledWindow({ shadow_type: Gtk.ShadowType.IN, >+ vexpand: true, >+ hexpand: true }); >+ content.add(this._sw); >+ this._sw.add(this._view); >+ >+ this.show_all(); >+ }, >+ >+ _onDefaultMode: function() { >+ }, >+ >+ _onRenameMode: function() { > } > }); > >@@ -770,8 +701,9 @@ const SelectionToolbar = new Lang.Class({ > toolbar.pack_end(this._toolbarProperties); > this._toolbarProperties.connect('clicked', Lang.bind(this, this._onToolbarProperties)); > >- // organize button >- this._toolbarCollection = new Gtk.Button({ label: _("Add to Collection") }); >+ // collections button >+ // if we are in Collections View, we should hide this button: see bug 665885. >+ this._toolbarCollection = new Gtk.Button({ label: _("Collections") }); > toolbar.pack_end(this._toolbarCollection); > this._toolbarCollection.connect('clicked', Lang.bind(this, this._onToolbarCollection)); > >@@ -881,9 +813,8 @@ const SelectionToolbar = new Lang.Class({ > > let dialog = new OrganizeCollectionDialog(toplevel); > >- dialog.widget.connect('response', Lang.bind(this, >+ dialog.connect('response', Lang.bind(this, > function(widget, response) { >- dialog.widget.destroy(); > Application.selectionController.setSelectionMode(false); > })); > }, >-- >2.1.4
Created attachment 305574 [details] [review] selections: new collections dialog
Created attachment 305585 [details] [review] application: add parameter_type when creating a GSimpleAction
Review of attachment 305585 [details] [review]: Thanks, Alessandro. This looks good to me. I pushed it after tweaking the commit message.
Created attachment 305741 [details] [review] selections: new collections dialog
Created attachment 306280 [details] [review] selections: new collections dialog
I just tried the latest patches. In general this looks really good - the key interactions are all right, and the dialog is already quite close to the mockups. As would be expected, there are some issues that need to be looked at: * Collections should be sorted according to the date/time created, with newest first. If there are a lot of collecitons and a new one is added, the list should scroll to the top, to ensure that it is visible. * With long collection names, the hamburger buttons are pushed off the right hand side of the dialog. The list should never scroll horizontally - instead, collection names should ellipsize to fit. * Renaming - when renaming a row, all the other rows become blank. This is wrong - they should still be displayed (possibly insensitive). * Renaming - the Esc key should cancel the rename (currently it doesn't do anything). * Deletion - undo row label should be left-justified. * Deletion - undo row should use a grey colour for the background, if possible. Thanks for your work so far, and let me know when it's ready to take another look!
Created attachment 308137 [details] [review] presentation: Extend objects instead of encapsulate them https://bugzilla.gnome.org/show_bug.cgi?id=752792
Created attachment 308822 [details] [review] selections: new collections dialog
Created attachment 309020 [details] [review] documents: Local collections are editable
Created attachment 309088 [details] [review] selections: new collections dialog
Created attachment 309091 [details] [review] selections: new collections dialog
Created attachment 309172 [details] [review] selections: new collections dialog
Created attachment 309212 [details] [review] selections: new collections dialog
Review of attachment 309212 [details] [review]: Thanks, Alessandro. The GdMainView in the collection view emits view-selection-changed when a collection is deleted from the dialog. This leads to the window title not being consistent with the rest of the application (as you showed at GUADEC). I think we should skip these spurious signals in _onViewSelectionChanged:view.js by comparing the view's mode with the mode of the application. ::: src/selections.js @@ +443,2 @@ + revealer.connect("notify::child-revealed", Lang.bind(this, this.deleteCollection)); + revealer.set_reveal_child(false); You can just write: revealer.reveal_child = false
Created attachment 309447 [details] [review] selections: new collections dialog I took the liberty to make the above changes.
Review of attachment 309447 [details] [review]: Some more comments: ::: src/selections.js @@ +378,3 @@ + _initDeleteView: function() { + let grid = new Gtk.Grid({ margin: 6, orientation: Gtk.Orientation.HORIZONTAL }); + let deleteLabel = new Gtk.Label({ label: '"' + this.collection.name + '" ' + _("removed"), We shouldn't split user-visible sentences. See: https://wiki.gnome.org/TranslationProject/DevGuidelines/Never split sentences We should use Unicode quotation marks instead of the ASCII ones. See: https://wiki.gnome.org/Design/OS/Typography @@ +620,3 @@ + let actionGroup = new Gio.SimpleActionGroup(); + let deleteAction = new Gio.SimpleAction({ name: 'delete-collection', parameter_type: GLib.VariantType.new('s') }); + let renameAction = new Gio.SimpleAction({ name: 'rename-collection', parameter_type: GLib.VariantType.new('s') }); These two lines are a bit too wide. @@ +623,3 @@ + actionGroup.add_action(deleteAction); + actionGroup.add_action(renameAction); + this.insert_action_group("dialog", actionGroup); Single quotes. @@ +643,2 @@ + this._collectionList = new CollectionList(); + this._collectionList.show(); I think it is better to call this.show_all() below and set the no-show-all property to TRUE for doneButton and cancelButton. @@ +773,3 @@ + this._headerBar.set_title(_("Rename")); + let children = this._headerBar.get_children(); + children.forEach(function(child) { child.show(); }); I think it is easier to read (and grep) if you use: this._cancelButton.show(); this._doneButton.show(); @@ +779,3 @@ + this._headerBar.set_title(C_("Dialog Title", "Collections")); + let children = this._headerBar.get_children(); + children.forEach(function(child) { child.hide(); }); Ditto.
Created attachment 309584 [details] [review] view: Remove duplicate signal connection
Created attachment 309585 [details] [review] view: Stop listening to info-updated when an item is removed Otherwise, newly created collections that were deleted from the dialog can reappear in the main application's view.
Created attachment 309598 [details] [review] selections: New collections dialog I fixed up the above issues.
Review of attachment 309598 [details] [review]: ::: src/selections.js @@ +655,3 @@ + // We should disconnect to the signal but if we try we get a segmentation fault + //this.disconnect(addId); + //this.disconnect(removeId); Did you try this._collectionList.disconnect(addId) ? :) @@ +656,3 @@ + //this.disconnect(addId); + //this.disconnect(removeId); + this.disconnect(this._keyPressEventId); This shouldn't be needed. Do we really need it?
Created attachment 309602 [details] [review] documents: Google collections aren't editable
Created attachment 309603 [details] [review] selections: new collections dialog
*** Bug 735863 has been marked as a duplicate of this bug. ***
Review of attachment 309603 [details] [review]: Much better. A few more comments: ::: src/selections.js @@ +580,3 @@ + return false; + + // we check if the name already exits: see bug 735863 This comment isn't needed. It is quite obvious what the following lines are for. :) @@ +762,3 @@ }, + _onRenameModeChanged: function() { We use the onFuncName format for callbacks, which this is not. Can you try to invent a different name? eg., you could use setRenameMode that takes a boolean parameter and sets this._renameMode and does the rest of the things. @@ +897,3 @@ + else + this._toolbarCollection.hide(); + })); I think we should continue to show the button if we are inside a collection, so that I can remove items from it or create a new collection with a subset of items. That means that the collection dialog should not let you delete the current active collection. I would suggest dropping this part from this patch because it isn't a big UI change, and we can continue to work on it after the UI freeze.
Created attachment 309609 [details] [review] selections: New collections dialog
Review of attachment 309609 [details] [review]: Thanks Alessandro. This looks good to me. ::: src/selections.js @@ +356,3 @@ + })); + let menu = new Gio.Menu(); + if (this.collection.canEdit()) As we already discussed on IRC, we are subtly changing the semantics of the canEdit vfunc here. Till now it was used to denote whether we can change the contents of a document (eg., a Google document). Now we are also using it to denote whether we can edit the metadata of a collection. It is particularly confusing because currently you can edit the contents of a Google document, but not its name. I won't mark this is a blocker because I am hesitant to explode the number of vfuncs we have, but this is something we should iron out in future. I am leaning towards letting the user change the names of remote items, because we already let them edit the contents of a Google document.
Thanks for all your hard work!
*** Bug 735867 has been marked as a duplicate of this bug. ***
*** Bug 745883 has been marked as a duplicate of this bug. ***
Thanks for all your work on this, Alessandro and Debarshi! It's so great to have this dialog in good shape, finally!
(In reply to Allan Day from comment #46) > Thanks for all your work on this, Alessandro and Debarshi! It's so great to > have this dialog in good shape, finally! I think there is a small niggle in the new dialog. When you enter the name of your first collection (ie. in the "empty state"), it takes a moment before the database gets updated. Meanwhile, the dialog looks strangely greyed out. We should somehow express this better. (We can continue on a separate bug if you want to.)
(In reply to Debarshi Ray from comment #47) ... > I think there is a small niggle in the new dialog. When you enter the name > of your first collection (ie. in the "empty state"), it takes a moment > before the database gets updated. Meanwhile, the dialog looks strangely > greyed out. > > We should somehow express this better. > > (We can continue on a separate bug if you want to.) Yes, a new bug would be good.