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 762305 - Ellipsize the route instructions in print layout
Ellipsize the route instructions in print layout
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-19 06:28 UTC by amisha
Modified: 2016-03-02 08:42 UTC
See Also:
GNOME target: ---
GNOME version: 3.19/3.20


Attachments
Comparison between 1 and 2 lines per instruction (436.08 KB, image/png)
2016-02-23 20:43 UTC, Razvan Brinzea
  Details
Ellipsize route instructions in print layout (1.70 KB, patch)
2016-02-23 20:44 UTC, Razvan Brinzea
none Details | Review
Ellipsize route instructions in print layout (1.84 KB, patch)
2016-02-24 20:33 UTC, Razvan Brinzea
none Details | Review
Ellipsize route instructions in print layout (1.59 KB, patch)
2016-03-02 08:15 UTC, Razvan Brinzea
committed Details | Review

Description amisha 2016-02-19 06:28:48 UTC
Overlapping of instructions is observed at places when instructions take more than 2 lines. http://i.imgur.com/tEwckla.png .
Comment 1 Jonas Danielsson 2016-02-19 08:22:48 UTC
So I think this should be solved by passing a new property "maxCharWidth" to the instructionRow widget. Then if that property is set we pass it to the label and make sure it ellipsize when passing that width. That can be done with the ellipsize property on GtkLabel: https://developer.gnome.org/gtk3/stable/GtkLabel.html#GtkLabel--ellipsize
Comment 2 Razvan Brinzea 2016-02-19 16:49:55 UTC
I wanted to look into this, but I'm not 100% sure I understand the issue. Do we want instruction labels to be ellipsized so they do not occupy more than 2 lines in the printed route?

It seems that instructionLabel in the instructionRow widget already has these properties:

<property name="ellipsize">end</property>
<property name="width_chars">20</property>
<property name="max_width_chars">20</property>
<property name="lines">3</property>

So anything larger than 3 lines actually gets ellipsized. I might have understood the issue wrongly, but shouldn't it be enough for us to overwrite the 'lines' property (and set it to 2) when creating InstructionRows from within printLayout?
Comment 3 Jonas Danielsson 2016-02-19 16:53:25 UTC
(In reply to Razvan Brinzea from comment #2)
> I wanted to look into this, but I'm not 100% sure I understand the issue. Do
> we want instruction labels to be ellipsized so they do not occupy more than
> 2 lines in the printed route?
> 
> It seems that instructionLabel in the instructionRow widget already has
> these properties:
> 
> <property name="ellipsize">end</property>
> <property name="width_chars">20</property>
> <property name="max_width_chars">20</property>
> <property name="lines">3</property>
> 
> So anything larger than 3 lines actually gets ellipsized. I might have
> understood the issue wrongly, but shouldn't it be enough for us to overwrite
> the 'lines' property (and set it to 2) when creating InstructionRows from
> within printLayout?

Right. That should probably work. I think I would prefer setting it to 1 though.
Comment 4 amisha 2016-02-19 20:46:58 UTC
(In reply to Jonas Danielsson from comment #3)
> (In reply to Razvan Brinzea from comment #2)
> > I wanted to look into this, but I'm not 100% sure I understand the issue. Do
> > we want instruction labels to be ellipsized so they do not occupy more than
> > 2 lines in the printed route?
> > 
> > It seems that instructionLabel in the instructionRow widget already has
> > these properties:
> > 
> > <property name="ellipsize">end</property>
> > <property name="width_chars">20</property>
> > <property name="max_width_chars">20</property>
> > <property name="lines">3</property>
> > 
> > So anything larger than 3 lines actually gets ellipsized. I might have
> > understood the issue wrongly, but shouldn't it be enough for us to overwrite
> > the 'lines' property (and set it to 2) when creating InstructionRows from
> > within printLayout?
> 
> Right. That should probably work. I think I would prefer setting it to 1
> though.

Yeah, that will work. Jonas i think setting it to 2 is a good idea, because it won't create issue of overlapping, plus it will be more informatory to user. Line 1 is almost covered up in displaying the direction. Road info gets more clearer in second line ,as far i have observed.
Comment 5 Razvan Brinzea 2016-02-23 20:43:25 UTC
Created attachment 322183 [details]
Comparison between 1 and 2 lines per instruction

This is a comparison between having one line per instruction row and having two lines per instruction row, for a sample route I usually use to test. I agree with Amisha, I think having just one line cuts off too much information.
Comment 6 Razvan Brinzea 2016-02-23 20:44:57 UTC
Created attachment 322184 [details] [review]
Ellipsize route instructions in print layout

This is the solution I've found. 'lines' is now an optional property that can be passed to the constructor of an InstructionRow, and it overrides the default setting of 3 lines that the InstructionRow labels have.
Comment 7 Jonas Danielsson 2016-02-24 08:56:40 UTC
Review of attachment 322184 [details] [review]:

Thanks!

My one concern with this is that we have a fixed height for instructionRow that we use in calculations.
Could you check with the inspector (GTK_DEBUG=insteractive gnome-maps) that the allocated height for a row with 1 lines and one with 2 lines is the same?

::: src/instructionRow.js
@@ +35,3 @@
     _init: function(params) {
         this.turnPoint = params.turnPoint;
+

extra new line

@@ +41,3 @@
+            this.lines = params.lines;
+            delete params.lines;
+        }

pls follow the same style we use elsewhere, this becomes:

this._lines = params.lines;
delete params.lines;

No need to check if it there this._lines becomes undefined if not there, and that is ok.
And we use _ to prefix internal variables.

This can even be:

this._lines = params.lines || 1;

And then you do not need an if later, right?

@@ +47,3 @@
+        if('lines' in this) {
+            this._instructionLabel.lines = this.lines;
+        }

Then this becomes just:

this._instructionLabel.lines = this._lines;
Comment 8 Jonas Danielsson 2016-02-24 08:56:55 UTC
(In reply to Razvan Brinzea from comment #5)
> Created attachment 322183 [details]
> Comparison between 1 and 2 lines per instruction
> 
> This is a comparison between having one line per instruction row and having
> two lines per instruction row, for a sample route I usually use to test. I
> agree with Amisha, I think having just one line cuts off too much
> information.

Thanks for this!
Comment 9 amisha 2016-02-24 18:02:12 UTC
Review of attachment 322184 [details] [review]:

Thanks Razvan.

::: src/instructionRow.js
@@ +41,3 @@
+            this.lines = params.lines;
+            delete params.lines;
+        }

Jonas, won't using params.lines||1 override the lines property in other cases too ,other than printing purpose?
i think it should be just: 
this._lines = params.lines;
delete params.lines;

if (this._lines)
    this._instructionLabel.lines = this._lines;
Comment 10 Jonas Danielsson 2016-02-24 18:05:50 UTC
Review of attachment 322184 [details] [review]:

::: src/instructionRow.js
@@ +41,3 @@
+            this.lines = params.lines;
+            delete params.lines;
+        }

Sure I thought the default value was 1 and then we could avoid an if-statement but you are right!
Comment 11 Razvan Brinzea 2016-02-24 20:33:35 UTC
Created attachment 322282 [details] [review]
Ellipsize route instructions in print layout

I cleaned up the code.
Also, I used the inspector to check the allocated size of InstructionRows, and in both cases (1 line and 2 lines per row), the allocated size is 328x48, so using 2 lines should not cause any problems.
Comment 12 Razvan Brinzea 2016-02-24 20:34:51 UTC
Also, I just noticed I slipped another newline, sorry about that. I seem to have missed it when using git diff.
Comment 13 amisha 2016-03-01 07:01:17 UTC
Review of attachment 322282 [details] [review]:

Thanks Razvan for the patch. Looks good enough. just little nits.

::: src/instructionRow.js
@@ +42,3 @@
         this.parent(params);
 
+        if(this._lines) {

Styling of if loop is as follows:
if (this._lines) {

::: src/printLayout.js
@@ +213,3 @@
             this._addSurface(surface, x, y, pageNum);
         }).bind(this));
+

extra line
Comment 14 Jonas Danielsson 2016-03-01 08:27:30 UTC
Review of attachment 322282 [details] [review]:

Thanks!

::: src/instructionRow.js
@@ +37,3 @@
         delete params.turnPoint;
 
+        this._lines = params.lines;

maybe just:

    let lines = params.lines?

I do not see that we need to have this variable as a member variable?
Comment 15 Razvan Brinzea 2016-03-02 08:15:00 UTC
Created attachment 322829 [details] [review]
Ellipsize route instructions in print layout

Route instructions are ellipsized after two lines
in order to prevent overlapping in the printed
route.
Comment 16 Jonas Danielsson 2016-03-02 08:38:03 UTC
Review of attachment 322829 [details] [review]:

Thanks!

I will commit this with some changes. But, please in the future make sure the patch is against _master_, this did not apply cleanly and I had to fix it up.
Comment 17 Jonas Danielsson 2016-03-02 08:41:47 UTC
Review of attachment 322829 [details] [review]:

pushed