Bug 48256

Summary:	clean out Hide/Show Cursor ...
Product:	LibreOffice	Reporter:	Michael Meeks <michael.meeks>
Component:	Spreadsheet	Assignee:	Eike Rathke <erack>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	caolanm, libreoffice, michael.meeks
Version:	Master old -3.6
Hardware:	Other
OS:	All
Whiteboard:	EasyHack,DifficultyBeginner,SkillCpp,TopicCleanup target:3.7.0
i915 platform:		i915 features:
Attachments:	Proposed patch Proposed patch Part 2 Proposed patch Part 3

Description Michael Meeks 2012-04-03 09:38:02 UTC

In the bad old days of calc, everything was rendered explicitly to the screen, and then the cursor was rendered over the top using XOR rendering. To get this right, before we rendered new things we would need to call:

HideCursor();
// do the incremental render
ShowCursor();

These days, we should never do that - all rendering should be done in response to an idle handler, and should paint from scratch instead of trying to be 'clever' like this (it will increasingly not work & just waste resources on  modern systems).

So - we need to audit the code for these Hide/Show pairs. Clearly some instances of hiding the cursor are prolly still applicable, but we should check all of them in sc/ 'git grep -5 HideCursor' - and remove and re-test all instances.

Potentially we want to add some calls to Invalidate() on the cursor region when this causes problems :-)

Thanks !

Comment 1 Florian Reisinger 2012-05-18 09:29:37 UTC

Deleted "Easyhack" from summary.

Comment 2 Thomas Arnhold 2012-06-08 21:01:25 UTC

Created attachment 62830 [details] [review]
Proposed patch

Comment 3 Thomas Arnhold 2012-06-08 21:03:52 UTC

Created attachment 62831 [details] [review]
Proposed patch Part 2

For my understanding CursorSwitcher does the same as Show/HideCursor only in the way that aCursorSwitch gets destructed after the method end (and so hidden).

Comment 4 Thomas Arnhold 2012-06-08 21:13:03 UTC

Created attachment 62832 [details] [review]
Proposed patch Part 3

Comment 5 Michael Meeks 2012-06-26 04:30:50 UTC

Looks lovely to me; if you did some testing & things seems to work fine - please do push them to master :-) it is possible that at some sites we might want to queue an idle re-draw of that area but presumably we can add that later if there are issues.

Thanks !

Comment 6 Caolán McNamara 2012-06-28 08:15:32 UTC

What's the situation with this patch, should it go in, or do you want someone to extra review it, or did something equivalent go in already ?

Comment 7 Thomas Arnhold 2012-06-29 20:48:22 UTC

Caolán: Yes it would be nice if someone else could review it.

Comment 8 Caolán McNamara 2012-06-29 23:14:47 UTC

caolanm->erack/kohei: one of you guys take this under your wing ?

Comment 9 Kohei Yoshida (inactive) 2012-07-02 10:48:15 UTC

(In reply to comment #8)
> caolanm->erack/kohei: one of you guys take this under your wing ?

IMO Michael's review in Comment 5 should be more than sufficient (since this is his EasyHack).  If Thomas needs extra assurance I can give mine.  Thomas, please push your changes to master.  Thanks!

Comment 10 Not Assigned 2012-07-13 11:15:06 UTC

Thomas Arnhold committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=76d07ffc5f87790865d9ea1a5c3c1093d0d01fe6

Resolves: fdo#48256 clean out Hide/Show Cursor

Comment 11 Caolán McNamara 2012-07-13 11:18:01 UTC

alright, so I pushed it. I'll leave it to the calc guys to fix things up if there's any problems with it :-)

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.