Opened 7 years ago

Closed 7 years ago

#4515 closed defect (fixed)

Palette interaction improvements

Reported by: marco Owned by: rwh
Priority: high Milestone: Update.1
Component: sugar Version:
Keywords: update.1? review+ Cc: Eben, marco, jg
Blocked By: Blocking:
Deployments affected: Action Needed:
Verified: no

Description

Interaction with palettes is currently really bad. Here is a list of fixes which I discussed with Eben, which I think would improve the situation a lot:

  • Ellipsis to long names
  • Show only when they are still
  • Bigger delay
  • Turn off scrubbing for floating
  • Hide palettes on view change
  • Crazy positioning on friends bar
  • Missing lower-left position
  • Scrubbing always reveal primary

I think we should considering doing all of these in a single batch for killjoy.

Attachments (6)

fix_palette_delay_and_motion.diff (6.1 KB) - added by rwh 7 years ago.
fix_friends_palette_positioning.diff (1.0 KB) - added by rwh 7 years ago.
fix_palette_delay_and_motion2.diff (3.7 KB) - added by rwh 7 years ago.
org.laptop.JournalActivity-1.log (8.4 KB) - added by marco 7 years ago.
fix_palette_tb.diff (831 bytes) - added by rwh 7 years ago.
fix_scrubbing_primary.diff (521 bytes) - added by rwh 7 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 7 years ago by marco

  • Priority changed from normal to high

comment:2 Changed 7 years ago by jg

  • Milestone changed from Never Assigned to XM - killjoy

comment:3 Changed 7 years ago by kimquirk

  • Milestone changed from Update.1 to Update.2

comment:4 Changed 7 years ago by Eben

  • Cc Eben added
  • Keywords update.1? added; killjoy? removed

The behavioral bugs on the palette interactions are a serious usability concern. I think we should really get a few of the items on the above list complete for update 1, if at all possible. In order of importance:

  1. show palette only when mouse remains still (below thresh)
  2. remove scrubbing from floating palettes
  3. friends bar positions (this is a blatant offense...the feature is mostly broken as is)
  4. scrubbing always reveal primary palettes only

If it's easier, I could accept temporarily ignoring #2 if #4 gets implemented.

comment:5 Changed 7 years ago by jg

  • Cc marco jg added

Marco, how much work? How loaded are you?

comment:6 Changed 7 years ago by marco

Eben, can I choose one between 2 and 4, depending on which one is easier? I'm not sure exactly before looking into it.

Jim, a few hours, I should be able to handle it.

comment:7 Changed 7 years ago by jg

  • Milestone changed from Update.2 to Update.1

comment:8 Changed 7 years ago by Eben

Yup. (1) is "mandadtory", you may choose either (2) or (4), and (3) would just be good to fix because it's downright broken.

comment:9 Changed 7 years ago by marco

  • Owner changed from marco to rwh

Changed 7 years ago by rwh

comment:10 Changed 7 years ago by rwh

These patches fix some problems: palette only shows up if mouse is moving slowly. 2 versions: 1 using notify-motion-event and the other by polling the cursor position. Since both of them require a timer I would recommend the second patch.
Working on fixing other issues with palettes

comment:11 Changed 7 years ago by rwh

Fix for friends palette positioning: get_allocation() returns values relative to the parent origin; might need fixes in more places.

comment:12 Changed 7 years ago by rwh

  • Keywords review? added

Marco, please have a look at these suggestions; maybe more to follow.

comment:13 Changed 7 years ago by marco

I agree we should poll cursor position, it makes the logic simpler and we don't gain anything with the other approach.

+        'mouse-slow': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE, ([])),
+        'mouse-fast': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE, ([])),

I'd use motion-slow/motion-fast.

+    _DISABLED = 1
+    _ENABLED = 2

Looks like we can do without these two states and set self._state to None in init and in stop().

+    _SIGNALLED_SLOW = 3
+    _SIGNALLED_FAST = 4

I'd rename these to _MOTION_SLOW and _MOTION_FAST

+        self._thresh2 = thresh**2

Rename to _threshold and keep the math all inside _detect_motion. It will make the code more clear and will not affect perf in any way.

+        self._need_cal = True

Let's initialize self._mouse_pos to None in the constructor instead and check for None in _detect_motion.

+        self._timeout_sid = gobject.timeout_add(self._delay, self._timer_cb)

Use _timeout_hid there, code is currently mixed but we settled on hid.

+#        print('_timer_cb(): md: %r, state: %r') % (self._motion_detected, self._state)

Please remove this before checking it in. We might use logging.debug but it would probably clutter the logs too much. Maybe turn it into a logging.debug and comment it out. Also 80 cols.

+        self._motion_detected = False   # Reset

We are generally not using "horizontal" comments. Sorry, a nitpick really :)

+        self._detect_motion()

It's not immediately clear this changes self._motion_detected. And it's not clear that motion_detected means fast motion. Also it doesn't seem to be necessary to store it. Can we do something like

fast_motion = self._dectect_motion()
if fast_motion...

comment:14 Changed 7 years ago by marco

  • Keywords review- added; review? removed

Changed 7 years ago by rwh

Changed 7 years ago by rwh

comment:15 Changed 7 years ago by rwh

  • Owner changed from rwh to marco

Pushed to master. I think this will fix most issues, but I'll look into other palette issues soon

Changed 7 years ago by marco

comment:16 Changed 7 years ago by marco

  • Owner changed from marco to rwh

Getting a trace (attached) when pressing "Back" in the journal, with 332. I'm not sure it's a regression but we should fix it before landing the patch in the Update.1 branch anyway.

Changed 7 years ago by rwh

comment:17 Changed 7 years ago by rwh

  • Keywords review? added; review- removed

Marco, I don't think there was regression; this behavior must have been there before. The patch I just attached fixes this by only calling get_origin if the window is not None. This does not seem to be the same thing as only calling get_origin if the widget is not NO_WINDOW.

comment:18 Changed 7 years ago by marco

We need to figure out why window is None there...

comment:19 Changed 7 years ago by marco

Btw stuff feels much better with these patches! We still need to do one between 2 and 4 in Eben list though.

comment:20 Changed 7 years ago by marco

  • Keywords review+ added; review? removed

The patch is fine as a band aid, please log a warning about it though. Ok to check in on master with that change.

comment:21 Changed 7 years ago by marco

I think the real problem is #5118, not necessary to fix it for Update.1 though.

comment:22 Changed 7 years ago by rwh

Pushed to master

comment:23 Changed 7 years ago by rwh

This patch only pops up the primary palette when scrubbing

comment:24 Changed 7 years ago by marco

+                state = group.get_state()
+                if state == self.SECONDARY:
+                    state = self.PRIMARY
+                self._set_state(state)

_set_state already check if the state is actually difference, so I think you can keep this simpler and do self._set_state(self.PRIMARY).

Please push on master, so we can get this one as well.

Changed 7 years ago by rwh

comment:25 Changed 7 years ago by rwh

You're right, that works as well. Patch updated and pushed to master.

comment:26 Changed 7 years ago by marco

  • Owner changed from rwh to ApprovalForUpdate

Seem to work pretty well on joyride 345.

comment:27 Changed 7 years ago by jg

  • Owner changed from ApprovalForUpdate to dgilmore

Approved.

comment:28 Changed 7 years ago by marco

  • Owner changed from dgilmore to rwh

#5351 is now that ticket which track getting this into the build. Reassigning to rwh to test once the rpm is in Update.1.

comment:29 Changed 7 years ago by marco

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

Verified in 657

Note: See TracTickets for help on using tickets.