GNOME Bugzilla – Bug 697701
zoom controls for users without scroll wheel
Last modified: 2013-08-16 12:48:15 UTC
The design currently has no visible controls for zooming/panning. I like this, but it presents a problem when I'm using the laptop without a mouse: there is no way to zoom out. The first thing I tried was middle-click-drag (IIRC Firefox calls this auto-scroll) but that does nothing and possibly isn't universally discoverable. keybindings might help but not really solve the problem.
(In reply to comment #0) > The design currently has no visible controls for zooming/panning. I like this, No you don't :) and we really need them soon. Design is very preliminary btw, Andreas and other designers have been way too busy with other stuff lately. > keybindings might help but not really solve the problem. Yeah, we need those too.
Zoom control designs coming up!
Created attachment 247151 [details] zoom controls
Created attachment 248609 [details] [review] ZoomControl implementation Hi, Here is a suggestion for implementation of the ZoomControl. It is sort of based on the approach of Mattias wip-branch. The styling is not my strong point so please think of ways to improve it. It bothers me a bit that the positioning (currently at [20, 20] but that's just something I choose for having a base for discussion) is done from the js file and not from the css or ui files. Does anyone know of a way of achieving that? Thankful for any comments and review that you have time for! Thanks!
Created attachment 248610 [details] [review] Implement keyboard bindings for zoom This patch adds simple key-pressed-event listener for ctrl-- and ctrl-+, do perform zoom. If we want zoom to be a menu item it is better to add keyboard shortcuts via action_groups I guess, but that is up to designers. Thanks!
Review of attachment 248610 [details] [review]: ::: src/mainWindow.js @@ +195,3 @@ + + if (state & Gdk.ModifierType.CONTROL_MASK) { + if (keyval == Gdk.KEY_plus) In general you want to use (===) instead of (==). (==) implies type coercion which most often is not what you want. Otherwise fine!
(In reply to comment #7) > Review of attachment 248610 [details] [review]: > > ::: src/mainWindow.js > @@ +195,3 @@ > + > + if (state & Gdk.ModifierType.CONTROL_MASK) { > + if (keyval == Gdk.KEY_plus) > > In general you want to use (===) instead of (==). (==) implies type coercion > which most often is not what you want. > Otherwise fine! Thanks for review! And for js knowledge :) New patch below.
Created attachment 248721 [details] [review] Implement keyboard bindings for zoom v2
Review of attachment 248609 [details] [review]: Nice work! But needs some more fiddling. Zeeshan: do you want to do a review also or are you fine with me ACKing when Jonas is done? ::: data/gnome-maps.css @@ +44,3 @@ +#zoom-out-button:insensitive { + background-image: url("zoom-out-insensitive.png"); +#zoom-in-button:hover { Most of this CSS code was made to try to mimick the designs without using images directly. And it was also very much not finished. :) So please try to remove the border-radius stuff and anything else that doesn't really matter when using images instead. :) ::: src/mapView.js @@ +172,3 @@ + let maxZoomLevel = this.view.get_max_zoom_level(); + let newZoomLevel = this.view.get_zoom_level() + steps; + this._zoom(-1); GObject properties can be accessed directly in GJS. Like this: let minZoomLevel = this.view.min_zoom_level; @@ +187,3 @@ + } else { + this._zoomControl.enableZoomIn(true); + if (newZoomLevel <= maxZoomLevel && Move the enable-/disable stuff to the zoom control and just do this.emit('zoom-end') ::: src/zoomControl.js @@ +33,3 @@ + Extends: GtkClutter.Actor, + + _init: function () { I think we should actually take a mapView instance here. @@ +48,3 @@ + this._zoomOutButton.connect('clicked', Lang.bind(this, function() { + this.emit('zoom-out'); + })); ...then drop these emit's and just call the map's zoomIn/zoomOut-methods. We should then also listen to 'zoom-in' and 'zoom-out' signals from mapview and update the disabled state in here instead I think. @@ +57,3 @@ + enableZoomOut: function(enabled) { + this._zoomOutButton.set_sensitive(enabled); + }, Please remove trailing comma! :)
Review of attachment 248721 [details] [review]: Looks good! Not sure if I can set accepted-commit-now though. Zeeshan?
Looked through the libchamplain code and found out that champlain actually notifies when the zoom-level change. You should be able to do: this._mapView.connect("notify::zoom-level", this._updateDisabled.bind(this)); ... or something similar.
(In reply to comment #10) > Review of attachment 248609 [details] [review]: > > Nice work! But needs some more fiddling. Zeeshan: do you want to do a review > also or are you fine with me ACKing when Jonas is done? > > ::: data/gnome-maps.css > @@ +44,3 @@ > +#zoom-out-button:insensitive { > + background-image: url("zoom-out-insensitive.png"); > +#zoom-in-button:hover { > > Most of this CSS code was made to try to mimick the designs without using > images directly. And it was also very much not finished. :) > So please try to remove the border-radius stuff and anything else that doesn't > really matter when using images instead. :) > Sure. But I would really like someone else to look at this, I don't really know what I'm doing with this. And the result I have now look different on different systems. I'll add a screenshot on how it looks at one system later. > ::: src/mapView.js > @@ +172,3 @@ > + let maxZoomLevel = this.view.get_max_zoom_level(); > + let newZoomLevel = this.view.get_zoom_level() + steps; > + this._zoom(-1); > > GObject properties can be accessed directly in GJS. Like this: > let minZoomLevel = this.view.min_zoom_level; Ok. > > @@ +187,3 @@ > + } else { > + this._zoomControl.enableZoomIn(true); > + if (newZoomLevel <= maxZoomLevel && > > Move the enable-/disable stuff to the zoom control and just do > this.emit('zoom-end') > Do we need to emit? > ::: src/zoomControl.js > @@ +33,3 @@ > + Extends: GtkClutter.Actor, > + > + _init: function () { > > I think we should actually take a mapView instance here. Ok. > > @@ +48,3 @@ > + this._zoomOutButton.connect('clicked', Lang.bind(this, function() { > + this.emit('zoom-out'); > + })); > > ...then drop these emit's and just call the map's zoomIn/zoomOut-methods. > > We should then also listen to 'zoom-in' and 'zoom-out' signals from mapview and > update the disabled state in here instead I think. Ok. (notify::zoom-level on mapView.view) > > @@ +57,3 @@ > + enableZoomOut: function(enabled) { > + this._zoomOutButton.set_sensitive(enabled); > + }, > > Please remove trailing comma! :) Ok.
Created attachment 248817 [details] [review] Implement ZoomControl v2.
Created attachment 248819 [details] Screenshot of my gnome-maps I can't seem to get rid of the border on this system. Is it theme related? How does it look for you? Anyone have any tips for the styling? Or can make me some svg-images for the different states? zoom-in zoom-out zoom-in-insensitive zoom-out-insensitive
(In reply to comment #13) > > @@ +187,3 @@ > > + } else { > > + this._zoomControl.enableZoomIn(true); > > + if (newZoomLevel <= maxZoomLevel && > > > > Move the enable-/disable stuff to the zoom control and just do > > this.emit('zoom-end') > > > > Do we need to emit? No I don't think so. Just listen to notify::zoom-level
(In reply to comment #13) > Sure. But I would really like someone else to look at this, I don't really know > what I'm doing with this. And the result I have now look different on different > systems. I'll add a screenshot on how it looks at one system later. I think we only need to care about Adwaita in this case. But yeah, I got stuck here too. I have no idea on how to: 1) implement this as a custom gtk+ css-style 2) where to find documentation on this
Created attachment 248985 [details] [review] Implement ZoomControl v3. So yeah. I ran in to problems with adwaita that i didn't have with my standard Ubuntu theme. In adwauta I had to clear the clutteractorbg, took a while before I caught that. I checked all possible style properties of GtkBox and GtkGrid before realizing. Also under adwaita I can't manage yo completely remove borders on the GtkButton. I've checked inner-border, default-border and default-outside-border, they are all 0 0 0 0 but still there is something there under adwaita. So this version of the patch uses GtkEventBox. The drawback(?) with that is that I can't seem to user :hover in the css. This version uses non-transparent, non-rounded images. The opacity and rounding is done in CSS. I don't think this should be commit as is. But rather some designer should look at making pixel-perfect images to use and update this. Or if this is commited and later fixed. Maybe. Or maybe this just get rejected and discarded as the rambling musings of an confused developer. And someone else implements proper sane controls. There are options. Much obliged.
Created attachment 248986 [details] [review] Implement ZoomControl v4 Remove stray context-style debug code.
Review of attachment 248986 [details] [review]: Overall fine. Don't worry about the styling for now. We obviously need help from someone here. ::: src/zoomControl.js @@ +42,3 @@ + + this._zoomInBox.connect('button-press-event', + Lang.bind(this, this._onZoomBoxClick)); Instead of this you can just do: this._zoomInBox.connect('button-press-event', Lang.bind(mapView, mapView.zoomIn)); @@ +44,3 @@ + Lang.bind(this, this._onZoomBoxClick)); + this._zoomOutBox.connect('button-press-event', + Lang.bind(this, this._onZoomBoxClick)); ...and then the same here. @@ +56,3 @@ + }, + + _onZoomBoxClick: function (widget, event) { And then remove this method altogether.
Created attachment 249280 [details] [review] Implement ZoomControl v5 Thanks!
Review of attachment 249280 [details] [review]: Looks good but we really want to use a GtkButton and not an EventBox. Could you please try getting jhbuild environment setup so you can have the latest Adwaita?
Review of attachment 248610 [details] [review]: ::: src/mainWindow.js @@ +195,3 @@ + + if (state & Gdk.ModifierType.CONTROL_MASK) { + if (keyval == Gdk.KEY_plus) Reference? I don't get any results from `git grep ===` in gnome-documents at least.
(In reply to comment #22) > Review of attachment 249280 [details] [review]: > > Looks good but we really want to use a GtkButton and not an EventBox. Could you > please try getting jhbuild environment setup so you can have the latest > Adwaita? I have a jhbuild environment setup and just copied my /opt/gnome/share/themes/Adwaita to ~/themes. Still the weird border around the button :(
(In reply to comment #23) > Review of attachment 248610 [details] [review]: > > ::: src/mainWindow.js > @@ +195,3 @@ > + > + if (state & Gdk.ModifierType.CONTROL_MASK) { > + if (keyval == Gdk.KEY_plus) > > Reference? I don't get any results from `git grep ===` in gnome-documents at > least. I don't have a reference, but a git grep in gnome-shell give quite a few occurrences. Btw, I could rework these two patches so that the keybindings can stand on it's own and could go in before the zoom-control one.
(In reply to comment #24) > (In reply to comment #22) > > Review of attachment 249280 [details] [review] [details]: > > > > Looks good but we really want to use a GtkButton and not an EventBox. Could you > > please try getting jhbuild environment setup so you can have the latest > > Adwaita? > > I have a jhbuild environment setup and just copied my > /opt/gnome/share/themes/Adwaita to ~/themes. Still the weird border around the > button :( I don't think you need any copying. Are you sure you are launching maps inside jhbuild environment? You could either do `jhbuild run gnome-maps` or `jhbuild shell` and then be in jhbuild env on the terminal from then on.. If you could provide the patch with button, I'll look into the theme issues.
(In reply to comment #26) > > I don't think you need any copying. Are you sure you are launching maps inside > jhbuild environment? You could either do `jhbuild run gnome-maps` or `jhbuild > shell` and then be in jhbuild env on the terminal from then on.. > > If you could provide the patch with button, I'll look into the theme issues. I do the jhbuild run gnome-maps thing. But I grant that there might be something wrong with my jhbuild setup, will look into it when I get some more time. Patch below.
Created attachment 250074 [details] [review] Implement ZoomControl v6
Comment on attachment 248721 [details] [review] Implement keyboard bindings for zoom v2 Patch commited with some changes, mainly that I added the implementation.
Created attachment 251687 [details] [review] Implement ZoomControl v7 So, the property that removed the funny border was: border-image-width. This patch is rebased against master and now uses the ChamplainView zoom functions. Any comments on the style and position of the controls would be welcome, maybe we can get this in to 3.10. Thanks.
Created attachment 251688 [details] [review] Implement ZoomControl v8 Sorry, git config mixup, correct email now.
Review of attachment 251688 [details] [review]: You fogot to add the zoomControl.js ?
Review of attachment 251688 [details] [review]: ::: src/gnome-maps.gresource.xml @@ +4,3 @@ <file preprocess="xml-stripblanks">app-menu.ui</file> <file preprocess="xml-stripblanks">main-window.ui</file> + <file preprocess="xml-stripblanks">zoom-control.ui</file> You forgot to add this file too. :) @@ +9,3 @@ + <file alias="zoom-out.png">../data/media/zoom-out.png</file> + <file alias="zoom-in-insensitive.png">../data/media/zoom-in-insensitive.png</file> + <file alias="zoom-out-insensitive.png">../data/media/zoom-out-insensitive.png</file> Are we still using these? If so, you want to addd these files to the patch.
Created attachment 251702 [details] [review] Implement ZoomControl v9 Added missing files. Sorry.
Created attachment 251703 [details] some small visual tweaks * smaller + and - * no rounding on bottom for the top button, no rounding on top for the bottom button * 1px padding instead of 2px
Created attachment 251710 [details] [review] Implement ZoomControl v10 Better?
Review of attachment 251710 [details] [review]: The commit log is obsolete as we ended-up not adding any zoomControl in previous patch. Suggestion: Add zoom-in/out controls ::: src/gnome-maps.gresource.xml @@ +9,3 @@ + <file alias="zoom-out.png">../data/media/zoom-out.png</file> + <file alias="zoom-in-insensitive.png">../data/media/zoom-in-insensitive.png</file> + <file alias="zoom-out-insensitive.png">../data/media/zoom-out-insensitive.png</file> These are still redundant. ::: src/zoomControl.js @@ +30,3 @@ + + _init: function (mapView) { + GtkClutter.Actor.prototype._init.call(this); Pardon my lack of JS skills but is this very indirect call really needed? Are you simply chaining up to parent's constructor? If so, you probably want to do it the same way as Application class does: this.parent({ application_id: 'org.gnome.Maps' });
(In reply to comment #37) > Review of attachment 251710 [details] [review]: > > The commit log is obsolete as we ended-up not adding any zoomControl in > previous patch. Suggestion: Add zoom-in/out controls > > ::: src/gnome-maps.gresource.xml > @@ +9,3 @@ > + <file alias="zoom-out.png">../data/media/zoom-out.png</file> > + <file > alias="zoom-in-insensitive.png">../data/media/zoom-in-insensitive.png</file> > + <file > alias="zoom-out-insensitive.png">../data/media/zoom-out-insensitive.png</file> > > These are still redundant. > Sorry, but in what way are they redundant? The files are in use, and referenced in the css as background images. If I don't add them here, then where should I add them? > ::: src/zoomControl.js > @@ +30,3 @@ > + > + _init: function (mapView) { > + GtkClutter.Actor.prototype._init.call(this); > > Pardon my lack of JS skills but is this very indirect call really needed? Are > you simply chaining up to parent's constructor? If so, you probably want to do > it the same way as Application class does: > > this.parent({ application_id: 'org.gnome.Maps' }); Changed it to this.parent(); I need to chain up before the clear-background thing.
(In reply to comment #38) > (In reply to comment #37) > > Review of attachment 251710 [details] [review] [details]: > > > > The commit log is obsolete as we ended-up not adding any zoomControl in > > previous patch. Suggestion: Add zoom-in/out controls > > > ::: src/gnome-maps.gresource.xml > > @@ +9,3 @@ > > + <file alias="zoom-out.png">../data/media/zoom-out.png</file> > > + <file > > alias="zoom-in-insensitive.png">../data/media/zoom-in-insensitive.png</file> > > + <file > > alias="zoom-out-insensitive.png">../data/media/zoom-out-insensitive.png</file> > > > > These are still redundant. > > > > Sorry, but in what way are they redundant? > The files are in use, and referenced in the css as background images. > If I don't add them here, then where should I add them? Sorry my bad, more like spliter's bad as it doesn't show the binary files. > > ::: src/zoomControl.js > > @@ +30,3 @@ > > + > > + _init: function (mapView) { > > + GtkClutter.Actor.prototype._init.call(this); > > > > Pardon my lack of JS skills but is this very indirect call really needed? Are > > you simply chaining up to parent's constructor? If so, you probably want to do > > it the same way as Application class does: > > > > this.parent({ application_id: 'org.gnome.Maps' }); > > Changed it to this.parent(); I need to chain up before the clear-background > thing. Cool, patch should be ready to push in next iteration then.
Created attachment 251785 [details] [review] Add zoom-in/zoom-out controls (v11) Here is latest. How is the visual looking, Andreas?
(In reply to comment #40) > Created an attachment (id=251785) [details] [review] > Add zoom-in/zoom-out controls (v11) > > Here is latest. > > How is the visual looking, Andreas? Looking good!
Hi, Andreas wanted to put the controls in the bottom right, but I have no idea how to do that using ClutterActors. Do anyone have an idea? I've tried the properties that looked relevant on ClutterActor, and even tried: this._zoomControl.set_position(this.view.get_width() - 20, this.view.get_height() - 20); But no go. I'm not familiar enough with Clutter to know how to do this.
My idea was that it would feel less intrusive, but if it's too hard, we can have it in the top-left for now.
Ok, lets get these controls in for now. Some thoughts: 1. I don't think controls should be in bottom right, they look out of the place there. 2. I wonder if we really need or even want to use clutter actor here. The sidebar (which is currently hidden because of lack of any content) use a gtk overlay so would be nice to use the same here. 3. The buttons don't seem to have a 'pressed' state, which is what a user will expect from a button. 4. For simple buttons like these, do we really need to use images for background? We could address all these concerns/action items in separate bug(s) now as this bug is really fixed now.