overlay-scrollbar

Merge lp://qastaging/~cimi/overlay-scrollbar/new-thumb-functionalities into lp://qastaging/overlay-scrollbar

new-thumb-functionalities
Merge into trunk

Proposed by Andrea Cimitan on 2011-10-12

Status:	Merged
Merged at revision:	311
Proposed branch:	lp://qastaging/~cimi/overlay-scrollbar/new-thumb-functionalities
Merge into:	lp://qastaging/overlay-scrollbar
Diff against target:	4177 lines (+1568/-1308) 5 files modified os/Makefile.am (+1/-1) os/os-bar.c (+454/-355) os/os-private.h (+63/-53) os/os-scrollbar.c (+1008/-866) os/os-thumb.c (+42/-33)
To merge this branch:	bzr merge lp://qastaging/~cimi/overlay-scrollbar/new-thumb-functionalities
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Ted Gould (community)		2011-10-12	Approve on 2011-10-17
Review via email: mp+79140@code.qastaging.launchpad.net

Description of the change

Big changes... are you really sure you want to review that? :-)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-10-12:

I do :)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-10-12:

From an usability point of view I'd reduce the value of TIMEOUT_THUMB_SHOW to 50ms. It seems better here.

Revision history for this message

Andrea Cimitan (cimi) wrote on 2011-10-12:

Marco, I'm thinking about increasing it to 150 :-) Please notice that the timeout is not respected when the thumb is touching the screen edge.

Anyway, the branch is still in progress :-) Not ready for review (see the "Status"!)

Revision history for this message

Ted Gould (ted) wrote on 2011-10-17:

In general, I'm a little uncomfortable with all the bit twittling for the state variable. I think that you should put that into its own function. Two reasons, it's easy to mess up. And it seems to be likely someplace you'll want to put a g_debug() statement when things are going wrong.

Also, the diffs would be a lot smaller if the name changes weren't included with the logic changes. I think next time you should split those out into two different merge requests.

Other than that, it was a big diff! :-) I didn't see anything on a general read through.

review: Approve