After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 731545 - Move out the initialization of the searchPopup
Move out the initialization of the searchPopup
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on:
Blocks:
 
 
Reported: 2014-06-12 03:22 UTC by Mattias Bengtsson
Modified: 2014-06-20 02:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
SearchPopup: forward popover properties (1.79 KB, patch)
2014-06-12 03:22 UTC, Mattias Bengtsson
committed Details | Review
Add PlaceEntry, a SearchEntry for places (5.74 KB, patch)
2014-06-12 03:22 UTC, Mattias Bengtsson
needs-work Details | Review
MainWindow: use PlaceEntry (6.86 KB, patch)
2014-06-12 03:22 UTC, Mattias Bengtsson
needs-work Details | Review
Add PlaceEntry, a SearchEntry for places (6.82 KB, patch)
2014-06-13 01:13 UTC, Mattias Bengtsson
none Details | Review
MainWindow: use PlaceEntry (7.60 KB, patch)
2014-06-13 01:17 UTC, Mattias Bengtsson
none Details | Review
Add PlaceEntry, a SearchEntry for places (6.92 KB, patch)
2014-06-13 02:12 UTC, Mattias Bengtsson
none Details | Review
MainWindow: use PlaceEntry (8.33 KB, patch)
2014-06-15 02:13 UTC, Mattias Bengtsson
committed Details | Review
Add PlaceEntry, a SearchEntry for places (6.95 KB, patch)
2014-06-16 03:29 UTC, Mattias Bengtsson
committed Details | Review

Description Mattias Bengtsson 2014-06-12 03:22:13 UTC
and searchEntry to a separate class called PlaceEntry and
use that instead.

This would be nice for the routing patches, but please don't
make one block on the other. I'll happily rebase either one
if the other gets merged first.
Comment 1 Mattias Bengtsson 2014-06-12 03:22:17 UTC
Created attachment 278307 [details] [review]
SearchPopup: forward popover properties

Take in a full properties object and forward that to GtkPopover instead
of hardcoding the properties to pass.
This makes searchPopup a bit more flexible in other settings.
Comment 2 Mattias Bengtsson 2014-06-12 03:22:23 UTC
Created attachment 278308 [details] [review]
Add PlaceEntry, a SearchEntry for places

PlaceEntry encapsulates all the search entry stuff in mainWindow in a
reusable way.
Comment 3 Mattias Bengtsson 2014-06-12 03:22:29 UTC
Created attachment 278309 [details] [review]
MainWindow: use PlaceEntry

Move over to using PlaceEntry in the HeaderBar.
Comment 4 Mattias Bengtsson 2014-06-12 06:26:06 UTC
Review of attachment 278308 [details] [review]:

::: src/placeEntry.js
@@ +18,3 @@
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Author: Mattias Bengtsson <mattias.jc.bengtsson@gmail.com>

This and the copyright notice should mention you too Jonas. Just realized.
Comment 5 Jonas Danielsson 2014-06-12 06:54:45 UTC
Review of attachment 278309 [details] [review]:

Nice refactor :)

::: src/mainWindow.js
@@ +56,1 @@
                                                     'search-completion',

Does the search-completion still needs to be in mainWindow?

@@ +74,3 @@
+        let placeEntry = this._createPlaceEntry(ui.searchCompletion)
+        ui.headerBar.set_custom_title(placeEntry);
+        placeEntry.has_focus = true;

Can't this be set when creating the PlaceEntry?

has_focus: true

?

@@ +86,3 @@
+    _createPlaceEntry: function(completion) {
+        completion.set_model(this._placeStore);
+        completion.set_match_func(PlaceStore.completionMatchFunc);

The completion stuff can be done from place-entry.ui / placeEntry.js?
Comment 6 Jonas Danielsson 2014-06-12 06:55:23 UTC
Review of attachment 278307 [details] [review]:

looks fine.
Comment 7 Jonas Danielsson 2014-06-12 06:58:03 UTC
Review of attachment 278308 [details] [review]:

I like it!

::: src/place-entry.ui
@@ +21,3 @@
+    </child>
+  </object>
+</interface>

Ok so you create this file, but do not add it or use it. I guess you forgot? We should be able to not have the completion in main-window.ui right?
And instead have it on the entry, avoiding duplication?

::: src/placeEntry.js
@@ +30,3 @@
+const SearchPopup = imports.searchPopup;
+
+const PlaceEntry = new Lang.Class({

Should it be PlaceSearchEntry? Not sure.
Comment 8 Jonas Danielsson 2014-06-12 06:58:22 UTC
Review of attachment 278308 [details] [review]:

I like it!

::: src/place-entry.ui
@@ +21,3 @@
+    </child>
+  </object>
+</interface>

Ok so you create this file, but do not add it or use it. I guess you forgot? We should be able to not have the completion in main-window.ui right?
And instead have it on the entry, avoiding duplication?

::: src/placeEntry.js
@@ +30,3 @@
+const SearchPopup = imports.searchPopup;
+
+const PlaceEntry = new Lang.Class({

Should it be PlaceSearchEntry? Not sure.
Comment 9 Mattias Bengtsson 2014-06-13 00:41:23 UTC
Review of attachment 278309 [details] [review]:

Thanks! :)

::: src/mainWindow.js
@@ +56,1 @@
                                                     'search-completion',

No, it should probably just be created in code, since that's the only gtkbuilder-stuff left anyways.

@@ +74,3 @@
+        let placeEntry = this._createPlaceEntry(ui.searchCompletion)
+        ui.headerBar.set_custom_title(placeEntry);
+        placeEntry.has_focus = true;

Yeah I tried that, but apparently setting has_focus before the child is added to a parent does nothing.

@@ +86,3 @@
+    _createPlaceEntry: function(completion) {
+        completion.set_model(this._placeStore);
+        completion.set_match_func(PlaceStore.completionMatchFunc);

Yeah, I'll just create it in code instead.
Comment 10 Mattias Bengtsson 2014-06-13 00:44:36 UTC
Review of attachment 278308 [details] [review]:

::: src/place-entry.ui
@@ +21,3 @@
+    </child>
+  </object>
+</interface>

Yeah, either actually use this or just construct it in code. I'll fix this.

::: src/placeEntry.js
@@ +30,3 @@
+const SearchPopup = imports.searchPopup;
+
+const PlaceEntry = new Lang.Class({

Yeah, makes sense.
Comment 11 Mattias Bengtsson 2014-06-13 01:13:05 UTC
Created attachment 278380 [details] [review]
Add PlaceEntry, a SearchEntry for places

PlaceEntry encapsulates all the search entry stuff in mainWindow in a
reusable way.

 - Construct the completion inside placeentry (via ui-file since it was easier).
 - Some small cleanups
Comment 12 Mattias Bengtsson 2014-06-13 01:15:39 UTC
Review of attachment 278308 [details] [review]:

::: src/placeEntry.js
@@ +30,3 @@
+const SearchPopup = imports.searchPopup;
+
+const PlaceEntry = new Lang.Class({

Actually I think we can just go with the shorter name. It's easy enough to understand and I hate reading code with unnecessarily long function- / variable names. 

Will change if you decide it's important though. :)
Comment 13 Mattias Bengtsson 2014-06-13 01:17:06 UTC
Created attachment 278381 [details] [review]
MainWindow: use PlaceEntry

Move over to using PlaceEntry in the HeaderBar.

 - Move the completion code to placeEntry
Comment 14 Mattias Bengtsson 2014-06-13 02:12:04 UTC
Created attachment 278383 [details] [review]
Add PlaceEntry, a SearchEntry for places

PlaceEntry encapsulates all the search entry stuff in mainWindow in a
reusable way.

 - Tweak alignment between searchEntry and popover.
Comment 15 Mattias Bengtsson 2014-06-15 02:13:01 UTC
Created attachment 278466 [details] [review]
MainWindow: use PlaceEntry

Move over to using PlaceEntry in the HeaderBar.

  - let the PlaceEntry constructor only take a props object.
Comment 16 Mattias Bengtsson 2014-06-16 03:29:26 UTC
Created attachment 278508 [details] [review]
Add PlaceEntry, a SearchEntry for places

PlaceEntry encapsulates all the search entry stuff in mainWindow in a
reusable way.

 - refine the width alignment, use the real allocation instead of the
   preferred width/height.
Comment 17 Jonas Danielsson 2014-06-19 07:21:29 UTC
Review of attachment 278466 [details] [review]:

(o/
Comment 18 Jonas Danielsson 2014-06-19 07:21:41 UTC
Review of attachment 278508 [details] [review]:

\o)
Comment 19 Mattias Bengtsson 2014-06-20 02:20:24 UTC
Attachment 278307 [details] pushed as ef095d2 - SearchPopup: forward popover properties
Attachment 278466 [details] pushed as 8d3a73b - MainWindow: use PlaceEntry
Attachment 278508 [details] pushed as 09d1dd3 - Add PlaceEntry, a SearchEntry for places
Comment 20 Mattias Bengtsson 2014-06-20 02:21:08 UTC
\o/