Bug 93889 - Extraordinary, un-necessary busy-loop removal ...
Summary: Extraordinary, un-necessary busy-loop removal ...
Status: REOPENED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-03 13:21 UTC by Michael Meeks
Modified: 2017-05-29 10:57 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2015-09-03 13:21:24 UTC
This one is a bit of a joke:

sc/source/ui/dbgui/sfiltdlg.cxx

sets up an idle handler that runs all the time:

    pIdle = new Idle;
    // FIXME: this is an abomination
    pIdle->SetPriority( SchedulerPriority::LOWEST );
    pIdle->SetIdleHdl( LINK( this, ScSpecialFilterDlg, TimeOutHdl ) );
    pIdle->Start();

The comment:

    // Hack: RefInput-Kontrolle
    pIdle = new Idle;

Is rather accurate ;-)

Reading the actual idle handler:

    // alle 50ms nachschauen, ob RefInputMode noch stimmt
    if( (_pIdle == pIdle) && IsActive() )
    {
        if( pEdCopyArea->HasFocus() || pRbCopyArea->HasFocus() )
        {
            pRefInputEdit = pEdCopyArea;
            bRefInputMode = true;
        }
        else if( pEdFilterArea->HasFocus() || pRbFilterArea->HasFocus() )
        {
            pRefInputEdit = pEdFilterArea;
            bRefInputMode = true;
        }
        else if( bRefInputMode )
        {
            pRefInputEdit = NULL;
            bRefInputMode = false;
        }
    }

Unless there is some cunning operator overloading or mis-use of the HasFocus() method somehow - then all of this could easily be renamed into a:

"SyncFocusState()"

method - and we can audit the code to find where we need to call that (ie. before we use the pRefInputEdit or bRefInputMode members.

Might be nice to prefix the member variables with 'm' or 'm_' to taste.

Then rip the idle handler out completely and all the muck that goes with it.

Worth checking the git annotate to see why that's there too I suspect =) perhaps there is a helpful bug report somewhere.

Thanks (for saving many people's batteries) =)
Comment 1 Robinson Tryon (qubit) 2015-12-10 11:40:59 UTC Comment hidden (obsolete)
Comment 2 Robinson Tryon (qubit) 2016-02-18 14:51:46 UTC Comment hidden (obsolete)
Comment 3 Steven Guo 2016-02-29 11:56:13 UTC
Jaskaran,

I was wondering if you're still working on this; if not, I'd like to make an attempt.
Comment 4 Jaskaran Singh 2016-03-03 14:45:27 UTC
I have committed a patch to fix this issue. Kindly, review it. 
Thanks.
Comment 5 Commit Notification 2016-03-04 09:44:34 UTC
Jaskaran committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6610ad9aee0c8299880cd1da6cd6a756860ccad9

tdf#93889 Remove a busy loop

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Jaskaran Singh 2016-03-17 08:31:00 UTC
Now since a patch has been successfully merged, Shouldn't this bug be marked RESOVLED?
Comment 7 Michael Meeks 2016-03-17 12:57:55 UTC
Sounds good to me - though I suspect there are more of these around the place =)
Comment 8 Commit Notification 2016-07-19 00:20:04 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=62442d9066ea553a4b68b8a93fa54748cbe96e06

Resolves: tdf#99360 Revert "tdf#93889 Remove a busy loop"

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 9 Eike Rathke 2016-07-19 00:23:14 UTC
That simply didn't work, see bug 99360.
Comment 10 Commit Notification 2016-07-19 07:13:11 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-5-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=fceea0ff4cfbacd552e8ac949f7db25d0e91493a&h=libreoffice-5-2

Resolves: tdf#99360 Revert "tdf#93889 Remove a busy loop"

It will be available in 5.2.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Commit Notification 2016-07-19 18:54:09 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-5-2-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ebddfbaeb71383ae94ff491da0b307c86c35e447&h=libreoffice-5-2-0

Resolves: tdf#99360 Revert "tdf#93889 Remove a busy loop"

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Guy Marcus 2017-05-21 20:48:07 UTC
I'm a new developer looking for a good easyhack to work on -- it is not clear to me from this comment chain if this bug is resolved or not. The last comment claims that a resolution is committed but reviewing the gerrit it doesn't seem to have been merged. Anyone with more information to clarify would be helpful!
Comment 13 Eike Rathke 2017-05-29 10:53:05 UTC
This one is not resolved. What you see above are commits that resolved bug 99360 by reverting the commit that claimed to "fix" this bug but caused the regression of bug 99360.
Comment 14 Eike Rathke 2017-05-29 10:57:09 UTC
Actually I tend to remove the easyHack flag from this.. the original hack exists because the problem which manifested also in bug 99360 was not easily solvable.