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 760251 - Can't half-maximize / snap app because window too wide
Can't half-maximize / snap app because window too wide
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
3.19.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on:
Blocks:
 
 
Reported: 2016-01-07 05:49 UTC by Hashem Nasarat
Modified: 2016-02-29 06:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for smaller PlaceEntry (1.73 KB, patch)
2016-02-25 18:39 UTC, elias
none Details | Review
Screencast after the proposed patch (1.32 MB, video/webm)
2016-02-26 07:56 UTC, elias
  Details
Screencast before the proposed patch (1.13 MB, video/webm)
2016-02-26 07:56 UTC, elias
  Details
Patch for smaller PlaceEntry (3.27 KB, patch)
2016-02-26 10:33 UTC, elias
none Details | Review
mainWindow: Allow PlaceEntry to shrink (2.02 KB, patch)
2016-02-26 11:48 UTC, elias
committed Details | Review
mainWindow: Increase margin around PlaceEntry (1.24 KB, patch)
2016-02-26 11:48 UTC, elias
committed Details | Review

Description Hashem Nasarat 2016-01-07 05:49:02 UTC
GNOME Shell allows apps to be dragged to the left/right edges of the screen and resized to fit exactly half the screen if the minimum width of the app is less than horizontal resolution / 2.

On my 1366x768 monitor (a very common size), the app can't be minimized narrow enough. I think it's because the PlaceEntry has a width-request of 500. Something smaller allows the app to be small enough but then the PlaceEntry doesn't take up enough space appropriately.

Also it'd be nice if there was more whitespace on either side of the PlaceEntry to allow users a target to click and drag to move the window around.

See pic for more explanation.

http://i.imgur.com/JjSGlzO.png
Comment 1 Razvan Brinzea 2016-02-14 08:26:37 UTC
A very simple fix would be resizing PlaceEntry. With a size of about 300, it could snap nicely to half of the screen, and it would also leave some more whitespace for dragging. Still, that would make it a bit too small for when Maps is full screen. 
Would there be any other recommended course of action?
Comment 2 elias 2016-02-25 18:39:59 UTC
Created attachment 322405 [details] [review]
Patch for smaller PlaceEntry

This uses a max_width_chars for a variable width. This allows the PlaceEntry to have about the same width as before if there is enough space. However, it is also possible to shrink the PlaceEntry along with the window. (I also increased the margin a bit.)
Comment 3 Hashem Nasarat 2016-02-26 02:00:17 UTC
Review of attachment 322405 [details] [review]:

Awesome! I'm really excited to have this worked on. 

(If you don't know, click [review] to see these comments inline)
For your commit message, a sentence of two for a "longer explanation" would be helpful: https://wiki.gnome.org/Git/CommitMessages

::: src/mainWindow.js
@@ +112,2 @@
     _createPlaceEntry: function() {
+        let placeEntry = new PlaceEntry.PlaceEntry({ mapView:         this._mapView,

While we're changing every line, I'd unalign the values in this dictionary (i.e. only have a single space after the ':' for every line). We typically don't align them so I'm not sure why they're aligned here.

@@ +115,3 @@
+                                                     margin_start:    35,
+                                                     margin_end:      35,
+                                                     max_width_chars: 53,

Let's aim to get the app to be less than 1366 / 2 = 683 pixels wide (to allow for 1366x768 screens). Smaller would be great too. To make the app as wide as possible make sure you have a route shown on the map that way the sidebar is open and the print button is shown in the toolbar.

I think when we have a route shown the app is wider than 683, can you get it to 683 or less?
Comment 4 Jonas Danielsson 2016-02-26 06:10:34 UTC
Review of attachment 322405 [details] [review]:

Thanks!

For the commit message, I would prefer the style of prefixing with the affected module.
Something like:
mainWindow: Allow PlaceEntry to shrink

I would like designer input on this. Could you do screencast or screenshot of before and after?

::: src/mainWindow.js
@@ +114,3 @@
+                                                     visible:         true,
+                                                     margin_start:    35,
+                                                     margin_end:      35,

Why do you also change the margins, this is an unrelated change. And a counter-productive one. If you leave the margin_start and margin_end at 6 you will be able to shrink the app more.

@@ +115,3 @@
+                                                     margin_start:    35,
+                                                     margin_end:      35,
+                                                     max_width_chars: 53,

Śo 53 chars. What does this mean? What affects char size? Does HDPI? Does font? Does width-char? Can we make this more obvious in some way?
Comment 5 elias 2016-02-26 07:55:03 UTC
I increased the margin because it was part of the bug report that it is hard to move the window around because there isn't any space to click on.

The max_width_chars is set to 53 because this results in a maximum width that is about the same size as 500px for me. It is affected by the font size but I don't think that's a problem. Even if the font is smaller, one is still able to get "the same amount of information" (the same number of visible characters) into the entry.

There might be a better way but I don't know how else we can get the something like a "max_width" property.
Comment 6 elias 2016-02-26 07:56:06 UTC
Created attachment 322438 [details]
Screencast after the proposed patch
Comment 7 elias 2016-02-26 07:56:38 UTC
Created attachment 322439 [details]
Screencast before the proposed patch
Comment 8 elias 2016-02-26 10:33:28 UTC
Created attachment 322448 [details] [review]
Patch for smaller PlaceEntry

The patch is splitted into two commits, the commit messages are improved and the values are unaligned.

gnome-builder also uses max_width_char for its search entry in the headerbar.
Comment 9 elias 2016-02-26 11:48:53 UTC
Created attachment 322452 [details] [review]
mainWindow: Allow PlaceEntry to shrink

The max_width_chars property allows us to set a maximum width
but the PlaceEntry is still able to shrink and thus the whole window.
Comment 10 elias 2016-02-26 11:48:59 UTC
Created attachment 322453 [details] [review]
mainWindow: Increase margin around PlaceEntry

The increased margin makes it easier to grab the window at the
headerbar.
Comment 11 Jonas Danielsson 2016-02-26 12:09:22 UTC
Review of attachment 322452 [details] [review]:

::: src/mainWindow.js
@@ +112,3 @@
     _createPlaceEntry: function() {
+        /* The max_width_chars property is set to 53 so the entry has about the
+         * same width as 500px while also being able to shrink.

People reading this will have no idea that it used to be 500, so writing "same as 500px" does not really make sense.

And it is about 500px with the default font? This feels so arbitrary :) Can't we instead have it as 50? To make a nice round number.
Comment 12 Jonas Danielsson 2016-02-26 12:09:23 UTC
Review of attachment 322452 [details] [review]:

::: src/mainWindow.js
@@ +112,3 @@
     _createPlaceEntry: function() {
+        /* The max_width_chars property is set to 53 so the entry has about the
+         * same width as 500px while also being able to shrink.

People reading this will have no idea that it used to be 500, so writing "same as 500px" does not really make sense.

And it is about 500px with the default font? This feels so arbitrary :) Can't we instead have it as 50? To make a nice round number.
Comment 13 Jonas Danielsson 2016-02-26 12:10:20 UTC
Review of attachment 322453 [details] [review]:

Thanks!

It is possible to drag while holding buttons, GTK+ makes sure of that, so is this really a problem?
Comment 14 elias 2016-02-26 12:46:44 UTC
Some users might hesitate to click on a button to move the window. Isn't that a decision for the design team?
Comment 15 elias 2016-02-26 12:56:21 UTC
I mixed up the videos for the screencasts. They are now corrected.

With the visible sidebar for routing the minimum width is still pretty wide but I am not sure which widget causes that. It is not the PlaceEntry.
Comment 16 Jonas Danielsson 2016-02-26 12:57:59 UTC
(In reply to elias from comment #14)
> Some users might hesitate to click on a button to move the window. Isn't
> that a decision for the design team?

I think it is a decision that was made by the design team... but not sure.
Comment 17 Jonas Danielsson 2016-02-26 12:58:34 UTC
(In reply to elias from comment #15)
> I mixed up the videos for the screencasts. They are now corrected.
> 
> With the visible sidebar for routing the minimum width is still pretty wide
> but I am not sure which widget causes that. It is not the PlaceEntry.

It is the max-width for the sidebar I would gather.
Comment 18 Andreas Nilsson 2016-02-26 14:06:28 UTC
(In reply to Jonas Danielsson from comment #16)
> (In reply to elias from comment #14)
> > Some users might hesitate to click on a button to move the window. Isn't
> > that a decision for the design team?
> 
> I think it is a decision that was made by the design team... but not sure.

Clicking and dragging on buttons should work, but I agree the empty space creates a bit more affordance [1], so I like the empty spaces that the patch adds.


1. https://en.wikipedia.org/wiki/Affordance (the article mentions Donald Normans brilliant book "The Design of Everyday Things, that's the best design book I know, and it's a very joyful read, read it today!")
Comment 19 Jonas Danielsson 2016-02-29 06:29:06 UTC
Review of attachment 322452 [details] [review]:

Thanks, will push with small changes!
Comment 20 Jonas Danielsson 2016-02-29 06:29:25 UTC
Review of attachment 322453 [details] [review]:

Thanks!