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 743128 - Displacement between tiles and grid
Displacement between tiles and grid
Status: RESOLVED FIXED
Product: gnome-2048
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-2048 maintainer(s)
gnome-2048 maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-01-18 15:53 UTC by Yanko Kaneti
Modified: 2015-02-20 23:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screenshot (24.05 KB, image/png)
2015-01-18 15:53 UTC, Yanko Kaneti
  Details
Move to grid layout (8.00 KB, patch)
2015-01-31 23:19 UTC, Yanko Kaneti
none Details | Review
Layout and resizing fixes (6.45 KB, patch)
2015-02-10 00:03 UTC, Yanko Kaneti
none Details | Review
patch v3 (5.87 KB, patch)
2015-02-11 13:47 UTC, Yanko Kaneti
none Details | Review

Description Yanko Kaneti 2015-01-18 15:53:35 UTC
Created attachment 294800 [details]
Screenshot

Attaching a screenshot with gnome-2048 running on fedora rawhide where you can see a small displacement between the tiles and the grey grid.

It becomes even more obvious when the window is resized. It seems like the grid doesn't resize at all with the tiles.
Comment 1 Yanko Kaneti 2015-01-31 23:19:22 UTC
Created attachment 295869 [details] [review]
Move to grid layout

I tried looking into this and ended up with this patch which moves everything to a grid layout and removes all the manual sizing.

But as a clutter newbie I have no idea tho what to do about the moving transitions, which stopped working during the process...
Comment 2 Juan R. Garcia Blanco 2015-02-04 21:35:17 UTC
Sorry for the delay. Yep, I also noticed this :(

I'm also a clutter newbie. A grid layout was my first attempt for creating the grid; however, I think I reached the same conclusion as you: it is not suitable if you want to move your actors around. In addition, you still need to maintain each tile position in order to move the existing tiles. At this point I think a grid layout is not suitable for this situation; please correct me if I'm wrong.

Right now I'm reworking the animations. I noticed also that sometimes animations seem to not complete; i.e. when a new tile appears it does not scale back to 1.0 after appearing. I hope this could be fixed after reworking the animations.
Comment 3 Yanko Kaneti 2015-02-10 00:03:59 UTC
Created attachment 296421 [details] [review]
Layout and resizing fixes

Took a second stab at this with a different apporoach and the result seems reasonable, basically it does a few unrelated things.

- Tie the tile sizes with a size constraint to an invisible actor that acts as a size controller (_mold). With this approach the animations still work as before.

- Tweak the rounded corner drawing, - this can be droppped

- Layout the tile text with a BinLayout with center alignment.

- Scale the font size to tile-size/4
Comment 4 Yanko Kaneti 2015-02-11 13:47:20 UTC
Created attachment 296597 [details] [review]
patch v3

Third attempt :)

- Retain the _mold sizing constraint from the previous patch
- Drop the rounded corner tweaking
- Redo the text size/position changes the proper way, by drawing it with pango+cairo.
Comment 5 Juan R. Garcia Blanco 2015-02-12 22:13:25 UTC
I've been looking into this and I do not see anything wrong in our code. I can easily get misalignment when resizing abruptly; however, if I switch to another application and back to 2048, the misalignment has gone. I think clutter may be missing a redraw.

Sorry, I've not tested the mold approach. I guess it works, but I do not see the reason. Honestly, since I do not see the reason why this is wrong, I'm reluctant to follow the mold approach. Instead, I've tested animating the resize operation, and by doing this the tiles seem to align perfectly. I'll push this change.

As for the text... why dropping ClutterText? I would definitely like to see text scaling wrt tile size and text size. I think in your patch you are not taking into account the size of the actual string, but only the size of the tile; correct me if I'm wrong.

If you agree, I think we can close this bug, and open a new one for the text scaling. If you can come up with a patch to scale text according to tile size and text size, without dropping ClutterText for a good reason, I'd be more than happy to apply it. Do you agree with this?
Comment 6 Juan R. Garcia Blanco 2015-02-12 22:29:08 UTC
Also, we should probably delay resizing, as it is done in https://github.com/GNOME/clutter/blob/master/examples/canvas.c
Comment 7 Yanko Kaneti 2015-02-12 23:23:41 UTC
(In reply to Juan R. Garcia Blanco from comment #5)
> Sorry, I've not tested the mold approach. I guess it works, but I do not see
> the reason. Honestly, since I do not see the reason why this is wrong, I'm
> reluctant to follow the mold approach. Instead, I've tested animating the
> resize operation, and by doing this the tiles seem to align perfectly. I'll
> push this change.

The idea of using constraints is to use the toolkit's instruments and avoid managing size tracking manually. If you don't see the value in that I am not going to try to convince you further.

> As for the text... why dropping ClutterText?

ClutterText has no notion of scaling.

> I would definitely like to see
> text scaling wrt tile size and text size. I think in your patch you are not
> taking into account the size of the actual string, but only the size of the
> tile; correct me if I'm wrong.

You are wrong ;) and prehaps didn't test the code at all. It definitely takes into account the size of the string, that's what the whole get_extents thing is about... 
 
> If you agree, I think we can close this bug, and open a new one for the text
> scaling. If you can come up with a patch to scale text according to tile
> size and text size, without dropping ClutterText for a good reason, I'd be
> more than happy to apply it. Do you agree with this?

You can proceed as you please. My opinion is that ClutterText is not suitable at all for the task at hand.
Comment 8 Yanko Kaneti 2015-02-13 08:17:09 UTC
FWIW I tested git master (at 1eb8a02) on rawhide and the displacement is still there. This is probably related to all the style changes in gtk+-3.15-devel which lead to allocation changes on the main screen happening a few more times after startup compared to 3.14.  So everything has to be wired up to react to those.
Comment 9 Juan R. Garcia Blanco 2015-02-13 22:13:42 UTC
I've been investigating this issue a bit more. I've noticed that, on my system, if I disable the text inside the tiles, they always align perfectly. Now I think the issue is resizing the text. Could you please try on your system to disable the text and see what happens?