Bug 52461 - Cell border highlight when selecting formula gives 2 or more different colors to same cell
Summary: Cell border highlight when selecting formula gives 2 or more different colors...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
3.6.0.0.beta2
Hardware: Other Windows (All)
: medium normal
Assignee: Rachit Gupta
QA Contact:
URL:
Whiteboard: target:4.3.0
Keywords: difficultyInteresting, easyHack, skillCpp, topicUI
Depends on:
Blocks:
 
Reported: 2012-07-24 20:14 UTC by Mikeyy - L10n HR
Modified: 2015-12-16 00:38 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Libreoffice (2.93 KB, image/png)
2012-07-24 20:14 UTC, Mikeyy - L10n HR
Details
Excel (2.96 KB, image/png)
2012-07-24 20:14 UTC, Mikeyy - L10n HR
Details
another example (4.18 KB, image/png)
2013-10-15 20:06 UTC, owezahra
Details
sine & root (7.06 KB, image/png)
2013-10-15 20:07 UTC, owezahra
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mikeyy - L10n HR 2012-07-24 20:14:14 UTC
Created attachment 64630 [details]
Libreoffice

Attached visual representation of problem.

When you have same cell in formula more then once, LO gives that cell as many colors as there are mentions in formula when it shoud give it same color for every reference inside formula.

Notice that cell E6 has 2 colors in LO, RED and GREEN, when in Excel (attachment no.2) it's always GREEN.
Comment 1 Mikeyy - L10n HR 2012-07-24 20:14:40 UTC
Created attachment 64631 [details]
Excel
Comment 2 owezahra 2013-10-15 20:06:01 UTC
Created attachment 87688 [details]
another example

I'm with you!
Especially when reviewing huge formulas (I'm an ElectricalEngineering Student) this really turns out as a mess, but even in small scales it is disappointing at least.
Comment 3 owezahra 2013-10-15 20:07:34 UTC
Created attachment 87689 [details]
sine & root

*real case*
Comment 4 Mikeyy - L10n HR 2013-11-11 06:27:32 UTC
Joel and Jan, sorry for bugging you, but I added you as CC of this bug because I saw you were QA in some other bugs I followed. Would this bug be good candidate for Easy Hacks?

Not really sure how much work is involved in fixing this bug, but hopefully one should "only" implement code which should check if cell is already present in formula and assign it same color as before.

I know it's not first priority bug since it's design bug, but maybe in easy hacks it will get more visiblity.
Comment 5 Jan Holesovsky 2013-11-11 10:01:17 UTC
So unfortunately it is not that easy; but still could be a nice advanced easy hack :-)

The color is set in two places:

sc/source/ui/app/inputhdl.cxx

search for 

aSet.Put( SvxColorItem( Color( ScRangeFindList::GetColorName( nCount ) ),

This sets the color of the text (=B1+B3).

sc/source/ui/view/gridwin4.cxx

search for

Color( ScRangeFindList::GetColorName( i ) ),

This sets the color of the higlighted cells.

In both cases, you can see that the ScRangeFindList::GetColorName() is being used; it is implemented here:

sc/source/ui/app/rfindlst.cxx

as

ColorData ScRangeFindList::GetColorName( size_t nIndex )
{
    return aColNames[nIndex % SC_RANGECOLORS];
}

You want to improve the implementation so that it does not blindly use the color based on the index, but actually it reuses the already used colors - so you want to make GetColorName non-static, traverse through the entries in the maEntries, and either use the same color (if the entry repeats), or a new one (if it is there for the 1st time).

Then you have to update the callers of GetColorName because it is not static any more.
Comment 6 Mikeyy - L10n HR 2013-11-11 13:53:09 UTC
(In reply to comment #5)
> So unfortunately it is not that easy; but still could be a nice advanced
> easy hack :-)

Thank you! Hopefully it will increase bug visibility.
Comment 7 Markus Mohrhard 2014-01-05 06:15:29 UTC
Let me take over the mentoring for this one from Kendy.

The solution is actually quite simple.

Use a map in ScRangeFindList for maEntries like std::map<ScRange, ScRangeFindData> and storing the color in ScRangeFindData. This also removes the need for the static function ScRangeFindList::GetColorName. Now your insert method does two things(actually std::map::insert will do the work for you):
If the range is already in the map return the old color otherwise insert and return the new color.

Bonus points for cleaning up gridwin4.cxx:794 and following.

Follow-up easy hack if interested could be to make ScInputHandler::InitRangeFinder use one of the existing formula parsers and go through the formula tokens instead of implementing our n-th formula parser.
Comment 8 Commit Notification 2014-02-08 21:32:31 UTC
Rachit Gupta committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=5ff8e1d8e31f23492ee1ccc3af0b73791cd5101b

fdo#52461 Multiple instances of same cell now get same color.



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 Caolán McNamara 2014-02-12 11:58:48 UTC
From the above commit, I'll assume this is fixed now and can be closed
Comment 10 Mikeyy - L10n HR 2014-02-12 16:38:56 UTC
Rachit, thank you for enhancement!
Please, don't forget to add your self to https://wiki.documentfoundation.org/ReleaseNotes/4.3

Tryed to install dev version from today (windows) but it has some kind of error and it's not possible to run it.
Will try again tomorrow.
Comment 11 Mikeyy - L10n HR 2014-02-13 21:02:06 UTC
Tested and it works. Thanks again!
Comment 12 Rachit Gupta 2014-02-14 07:11:42 UTC
Mikeyy, my pleasure :)
Comment 13 Joel Madero 2014-02-25 16:12:49 UTC
Indeed - but I'm going to open a new request that perhaps Rachit would be interested in.

Currently the colors only show when you leave the cell and go back to it, not live. I'll explain on a new bug report and cc Rachit :)
Comment 14 Óvári 2015-06-29 23:56:16 UTC
Please see:
https://bugs.documentfoundation.org/show_bug.cgi?id=92426

Thank you
Comment 15 Óvári 2015-06-30 02:21:22 UTC
Please see for related:
https://bugs.documentfoundation.org/show_bug.cgi?id=92441

Thank you
Comment 16 Rachit Gupta 2015-06-30 06:30:22 UTC
Hello everyone.

As much as I would love to work on the issue, I would not be able to do so because now I am busy with my corporate job.

If anyone wishes to take over this issue, please go ahead. I will still look into it if and when I get the time.

I apologize for the inconvenience.

Regards,
Rachit
Comment 17 Robinson Tryon (qubit) 2015-12-16 00:38:21 UTC
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyInteresting SkillCpp TopicUI )
[NinjaEdit]