Opened 7 years ago

Closed 6 years ago

#3944 closed defect (fixed)

Neighbourhood view icons can collide with central buddy icon

Reported by: garycmartin Owned by: humitos
Priority: High Milestone: Unspecified
Component: Sugar Version: 0.97.x
Severity: Major Keywords: 13.1.0, regression
Cc: manuq, humitos Distribution/OS: Unspecified
Bug Status: Unconfirmed

Description

In the gtk3 version of the Sugar shell, Neighbourhood view icons can now sometimes collide with the central buddy icon, they should avoid overlapping with this icon (as well as each other). See attached screen shot (though I've seen much worse, just didn't managed to get a screen shot at the time).

Attachments (7)

gtk3_neighbourhood_icons_can_collide_with_owner_buddy.png (19.6 KB) - added by garycmartin 7 years ago.
testspreadlayout.py (1.3 KB) - added by manuq 7 years ago.
TestCase, copy to sugar/src/jarabe/desktop/ and run from a sugar shell.
0001-Avoid-collisions-with-central-buddy-SL-3944.patch (1.4 KB) - added by humitos 7 years ago.
This patch reduces the probability to collide with the central buddy (locked position). It marks the icon to be re-arranged if it collides with the central buddy.
0002-Do-not-check-intersection-with-the-same-icon.patch (1.3 KB) - added by humitos 7 years ago.
A little improved version of _detect_collisions
collision.patch (2.5 KB) - added by dsd 7 years ago.
improved collision detection behaviour
collision1.png (52.6 KB) - added by manuq 7 years ago.
Testing improved algorithm
collision2.png (22.4 KB) - added by manuq 7 years ago.
Testing improved algorithm 2

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by manuq

  • Owner changed from erikos to manuq
  • Status changed from new to assigned

comment:2 Changed 7 years ago by manuq

  • Keywords regression added

comment:3 Changed 7 years ago by manuq

  • Priority changed from Unspecified by Maintainer to High
  • Severity changed from Unspecified to Major

Changed 7 years ago by manuq

TestCase, copy to sugar/src/jarabe/desktop/ and run from a sugar shell.

Changed 7 years ago by humitos

This patch reduces the probability to collide with the central buddy (locked position). It marks the icon to be re-arranged if it collides with the central buddy.

Changed 7 years ago by humitos

A little improved version of _detect_collisions

comment:4 Changed 7 years ago by humitos

  • Cc humitos added
  • Owner changed from manuq to humitos
  • Status changed from assigned to accepted

I've just uploaded a patch that mark the icon to be re-arranged if it collides with the central buddy (locked icon). This reduces the possibility to collide with it.

I tried many times the test case that manuq uploaded some weeks ago and I think the patch it's acceptable. Sometimes I get the icons collided by that it's a normal behaviour because of the way that the SugarExt.Grid is implemented.

Well, this patch needs testing because maybe I am skipping some possibilities.

comment:5 Changed 7 years ago by dsd

humitos has figured out whats going wrong here, but I don't think the patch really approaches the issue in a way that can get a good review. I'll try to help with the explanation:

Two icons are colliding: the owner icon, and an AP icon being added. The owner icon is stored on the grid as locked (it should not be move), the AP icon is not locked (it can be moved). The collision detection code here is failing to resolve the collision.

_detect_collisions() is called for the AP icon. It iterates over each of the other children in the grid, checking for collisions.

It reaches the owner icon, and this happens:

            if c != child and intersection.width > 0:

This condition passes: the owner is not the AP icon, and an intersection(collision) was found.

                if (c not in self._locked_children and
                    c not in self._collisions):
                    collision_found = True

This condition fails: c(the owner) is locked, so we never record a collision as happening. And that is the only place where a collision would possibly be recorded. So we take no action on the collision, resulting in the behaviour (colliding icons) that we see here.

Maybe a simple and logical fix can be found to this that would be more suitable for this late stage of 0.98. Failing that, I'm attaching my take on redoing this logic in a way that is easy to understand, inclusive of this case, and heavily commented. Its not tested.

Changed 7 years ago by dsd

improved collision detection behaviour

comment:6 Changed 7 years ago by humitos

comment:7 Changed 7 years ago by erikos

  • Keywords r? added

Changed 7 years ago by manuq

Testing improved algorithm

Changed 7 years ago by manuq

Testing improved algorithm 2

comment:8 Changed 7 years ago by manuq

I have been testing the improved code for some days now. I can still find some overlap with the central buddy's head as the screenshots show, but is minor, and I think this is an improvement. In the random view you can force the collision dragging activity icons into the central buddy. They are moved to the outside now (there is the jump described in #3960). I would say let's push and improve further later.

comment:9 Changed 7 years ago by dsd

I think humitos' 0001-Avoid-collisions-with-central-buddy-SL-3944.patch is a good solution for 0.98. It means that when a collision is found with a locked icon, instead of not moving any icon, the unlocked one is moved. The logic is hard to follow, but that can be fixed in the next cycle with my rewrite above.

comment:10 Changed 7 years ago by dsd

note: I haven't tested either humitos' patch or my rewrite.

comment:11 Changed 7 years ago by manuq

Pushed the Avoid... as d0a5c110 but let's keep this open for further improvement.

comment:12 Changed 7 years ago by erikos

  • Keywords r? removed

comment:13 Changed 7 years ago by erikos

I can still see occasions where an AP icon can collide with the owner icon, tested with os32, hence Sugar 0.98.3.

comment:14 Changed 6 years ago by dnarvaez

  • Milestone changed from 0.98 to Unspecified

comment:15 Changed 6 years ago by dnarvaez

  • Resolution set to fixed
  • Status changed from accepted to closed

It seems like the patch improves the situation a lot. If it still happens rarely I think it's not such a big deal, compared to the other collab issues we have.

Note: See TracTickets for help on using tickets.