Ticket #4515 (closed defect: fixed)

Opened 7 years ago

Last modified 7 years ago

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
Action Needed: Verified: no
Deployments affected: Blocked By:
Blocking:

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

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 (0.8 kB) - added by rwh 7 years ago.
fix_scrubbing_primary.diff (0.5 kB) - added by rwh 7 years ago.

Change History

Changed 7 years ago by marco

  • priority changed from normal to high

Changed 7 years ago by jg

  • milestone changed from Never Assigned to XM - killjoy

Changed 7 years ago by kimquirk

  • milestone changed from Update.1 to Update.2

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.

Changed 7 years ago by jg

  • cc marco, jg added

Marco, how much work? How loaded are you?

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.

Changed 7 years ago by jg

  • milestone changed from Update.2 to Update.1

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.

Changed 7 years ago by marco

  • owner changed from marco to rwh

Changed 7 years ago by rwh

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

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.

Changed 7 years ago by rwh

  • keywords review? added

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

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...

Changed 7 years ago by marco

  • keywords review- added; review? removed

Changed 7 years ago by rwh

Changed 7 years ago by rwh

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

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

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.

Changed 7 years ago by marco

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

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.

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.

Changed 7 years ago by marco

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

Changed 7 years ago by rwh

Pushed to master

Changed 7 years ago by rwh

This patch only pops up the primary palette when scrubbing

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

Changed 7 years ago by rwh

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

Changed 7 years ago by marco

  • owner changed from rwh to ApprovalForUpdate

Seem to work pretty well on joyride 345.

Changed 7 years ago by jg

  • owner changed from ApprovalForUpdate to dgilmore

Approved.

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.

Changed 7 years ago by marco

  • status changed from new to closed
  • resolution set to fixed

Verified in 657

Note: See TracTickets for help on using tickets.