Bug Hunting Session
Bug 34425 - [EasyHack] formatting background color toolbar button
Summary: [EasyHack] formatting background color toolbar button
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
3.3.0 release
Hardware: All All
: medium enhancement
Assignee: Not Assigned
URL:
Whiteboard: color_handling
Keywords: difficultyBeginner, easyHack, skillCpp, topicUI
: 34178 38813 46069 (view as bug list)
Depends on:
Blocks: 36646 45671
  Show dependency treegraph
 
Reported: 2011-02-17 23:00 UTC by Winfried Donkers
Modified: 2015-12-16 00:12 UTC (History)
11 users (show)

See Also:
Crash report or crash signature:


Attachments
button showing last used background color (red) (1.32 KB, image/png)
2011-02-17 23:00 UTC, Winfried Donkers
Details
code mods so far on 34425 (1.90 KB, application/gzip)
2011-12-15 23:04 UTC, Winfried Donkers
Details
modified code, still not complete (7.49 KB, text/plain)
2011-12-21 00:00 UTC, Winfried Donkers
Details
modified code so far, still not complete (7.95 KB, patch)
2011-12-27 02:58 UTC, Winfried Donkers
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Winfried Donkers 2011-02-17 23:00:08 UTC
Created attachment 43505 [details]
button showing last used background color (red)

The toolbar button for background color shows the last used color. It would be greatly appreciated if it were possible that with a single click this color can be used to fill the background of another cell.
Now the color shown is misleading (has no function) and it is a lot of extra work to select the color each time.
(Split the button in last used color and selection from pallette?)
Comment 1 Winfried Donkers 2011-08-17 04:54:52 UTC
The problem applies to swriter as well (background colour).
Also, swriter seems to have a solution: the buttons for font colour and marking colour are split (left part of the button gives the last used colour to the selected text and right part of the button produces the pallette to choose from).
I hope this information will help you.
Comment 2 Björn Michaelsen 2011-11-22 14:52:13 UTC
EasyHackifying.
Comment 3 Björn Michaelsen 2011-11-23 04:16:39 UTC
I assume that to be the class svx::ExtrusionColorControl:

http://opengrok.libreoffice.org/xref/core/svx/inc/svx/extrusioncolorcontrol.hxx#38

HTH for starters, more later.
Comment 4 Björn Michaelsen 2011-11-24 04:57:35 UTC
Here are some backtraces showing what triggers the color update in sw:
FontColor:
#0  svx::ToolboxButtonColorUpdater::Update (this=0x18ff350, rColor=rgb(0, 0, 255)) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/svx/source/tbxctrls/tbxcolorupdate.cxx:78
#1  0x00007fffdc365df5 in SvxFontColorExtToolBoxControl::StateChanged (this=0x1901bf0, nSID=10537, eState=32, pState=0x1a4b9e0) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/svx/source/tbxctrls/tbcontrl.cxx:2325
#2  0x00007ffff5956cad in SfxToolBoxControl::statusChanged (this=0x1901bf0, rEvent=...) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/sfx2/source/toolbox/tbxitem.cxx:633

Background:
#0  svx::ToolboxButtonColorUpdater::Update (this=0x1901f60, rColor=rgb(0, 0, 255)) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/svx/source/tbxctrls/tbxcolorupdate.cxx:78
#1  0x00007fffdc36557e in SvxColorToolBoxControl::StateChanged (this=0x1905f90, eState=32, pState=0x19f7ec0) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/svx/source/tbxctrls/tbcontrl.cxx:2225
#2  0x00007ffff5956cad in SfxToolBoxControl::statusChanged (this=0x1905f90, rEvent=...) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/sfx2/source/toolbox/tbxitem.cxx:633
#3  0x00007ffff5799ff6 in SfxDispatchController_Impl::StateChanged (this=0x1902e30, nSID=10185, eState=32, pState=0x19d18d0, pSlotServ=0x1903418)

And Cell Background in sc:
#0  svx::ToolboxButtonColorUpdater::Update (this=0x1c83e40, rColor=rgb(0, 0, 255)) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/svx/source/tbxctrls/tbxcolorupdate.cxx:78
#1  0x00007fffdc364eca in SvxFontColorToolBoxControl::StateChanged (this=0x1c83fc0, eState=32, pState=0x1a180b0) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/svx/source/tbxctrls/tbcontrl.cxx:2155
#2  0x00007ffff5956cad in SfxToolBoxControl::statusChanged (this=0x1c83fc0, rEvent=...) at /mnt/striped/bjoern/.jenkins/jobs/libreoffice-master/workspace/sfx2/source/toolbox/tbxitem.cxx:633

So the SvxFontColorExtToolBoxControl/SvxColorToolBoxControl classes are actually better starting points.
Comment 5 Winfried Donkers 2011-11-24 05:05:22 UTC
(In reply to comment #4)
> Here are some backtraces ...
> So the SvxFontColorExtToolBoxControl/SvxColorToolBoxControl classes are
> actually better starting points.
Thank you for these pointers, I'll set my teeth in them (extrusioncolorcontrol did help me much, I must admit).
Comment 6 Björn Michaelsen 2011-11-24 05:51:20 UTC
Does it maybe make sense to replace all SvxFontColorToolBoxControls (which opens
the popup on click) with SvxFontColorExtToolBoxControl (which applies the last
color)?

Sorry about extrusioncolorcontrol, it was just a quick guess (and misleading indeed).
Comment 7 Winfried Donkers 2011-11-24 07:53:19 UTC
(In reply to comment #6)
> Does it maybe make sense to replace all SvxFontColorToolBoxControls (which
> opens
> the popup on click) with SvxFontColorExtToolBoxControl (which applies the last
> color)?

As far as can see (in a very short time), a new class SvxColorExtToolBoxControl needs te be derived from SvxColorToolBoxControl. 
It'll take some time, finding out the details of SvxFontColorExtToolBoxControl and SvxFontColorToolBoxControl and SvxColorToolBoxControl to create a proper SvxColorExtToolBoxControl. 
Don't expect results from me in a very short time :)
 
> Sorry about extrusioncolorcontrol, it was just a quick guess (and misleading
> indeed).
No problem, always interesting to see something new :)
Comment 8 Björn Michaelsen 2011-11-25 11:48:22 UTC
> Don't expect results from me in a very short time :)

No problem ;)

Just dont hesitate to ask, if you need further hints. And if I dont answer immediately, I might just be frantically fixing a packaging problem for Ubuntu. But #libreoffice on freenode should help you out in that case.
Comment 9 Winfried Donkers 2011-12-05 01:30:10 UTC
(In reply to comment #8)
> Just dont hesitate to ask, if you need further hints. 

Still working on it. The compiling and linking and debug-attempts made me wish for a faster computer, which I now have ordered (I don't want you to be waiting to long for me :)).
I have created a SvxColorExtToolBoxControl class which possibly works OK, but:
-does it need a new ID (e.g. SID_BACKGROUND_COLOR_EXT instead of SID_BACKGROUND_COLOR in /core/svx/inc/svx/svxids.hrc)?
-in /core/svx/source/tbxctrls/tbxcolorupdate.cxx, ToolboxButtonColorUpdater::Update is called whenever you select/enter another cell, so that I cannot test pressing the left part of the toolbutton (apply last used background color). The toolbutton-color should be updated only when selecting a new backgroundcolor from the palet (right part of the toolbutton. Do you have hints where/how to disable updating the toolbutton color?
-I see lot of german comments. Is it wise to translate these in the same patch (should I succeed), or should I commit two patches (one bug and one translation)?
Comment 10 Winfried Donkers 2011-12-13 04:20:27 UTC
> Just dont hesitate to ask, if you need further hints. 

I'm stuck. The toolbar in scalc now uses SvxFontColorExtToolBoxControl (for background and font color) which has 'double action'(apply color as shown on button bitmap / choose from color palettte), but:
When you select a cell, the color shown on button bitmap is updated to the color used in the cell. I need the color shown on button bitmap to change only when a color has been choosen from the color palette, but I can't find a way. Neither do I know where this 'change button bitmap color' is called when you select a cell.
SvxFontColorExtToolBoxControl::StateChanged() is called, but I can't differentiate between a caal originating from the button itself (color chosen from palette)and from selecting a cell.

In swriter it works (the button bitmap color is not changed when you select text with a different color or place your cursor somewhere and the last used color is applied to a selection when you press the (left side) button.

help and or further hints would be much appreciated!
Comment 11 Larsen 2011-12-15 01:03:54 UTC
*** Bug 34178 has been marked as a duplicate of this bug. ***
Comment 12 Jan Holesovsky 2011-12-15 15:55:07 UTC
Winfried: Glad to hear about the progress so far! :-)

Please - can you attach here the patch you have so far?  An output of 'git diff' is perfectly OK for now - so that one can see exactly what you have done.  That will be the easiest way to help to unstuck you.
Comment 13 Winfried Donkers 2011-12-15 23:04:56 UTC
Created attachment 54485 [details]
code mods so far on 34425
Comment 14 Winfried Donkers 2011-12-15 23:05:20 UTC
(In reply to comment #12)
> Please - can you attach here the patch you have so far?  An output of 'git
> diff' is perfectly OK for now - so that one can see exactly what you have done.
>  That will be the easiest way to help to unstuck you.

Jan,

Thanks for offering help!

Basically I have 3 problems:
-not familiar with the LibO code (structure). That why I started on this easyhack :)
-the button bitmap color changes when you select another cell. it should change only when you select another color from the pallette. I do not know how to differentiate between the two in tbcontrl.cxx, nor where the call comes from (in the case of selecting a cell)
-the left hand button does not change the background color (or font color in case of the other button). I haven't found out yet how the mechanism works.

I have changed the code in a pre-patch-style (make changes that can be undone easily, work with trial and error, use fixed values for test (e.g the bitmap color in the current diff), use printf() to see when what happens.
I am not familiar with gdb, kdbg goes better, but my LibO-development machine at work is slow and at home I have a new machine which I am still setting up. I hope to be able to spend some time this weekend on it.

It's a learning process but some hints do help to get myself unstuck ;)
Currently I want to find out how the SID_..._EXT is connected to the ...ExtToolBoxControl as they are registered with SID_... .
Comment 15 Winfried Donkers 2011-12-21 00:00:18 UTC
Created attachment 54626 [details]
modified code, still not complete

Attached diff shows the present state of my labour.
One problem has been solved: the bitmap color in the buttons changes only when a color from the palette is chosen, not when a cell with another color is selected.
The other problem still stands: pressing the left-hand part of the button (which should apply the color right away) still has no effect at all. None of the Execute-functions in scdll.cxx is called. I do not know how to 'connect' pressing the button to StateChanged() in tbContrl.cxx (with the mark that the color should be applied without dialog).

Comments and hints are welcome. In the means time I keep searching and experimenting.
Comment 16 Björn Michaelsen 2011-12-23 11:49:07 UTC
[This is an automated message.]
This bug was filed before the changes to Bugzilla on 2011-10-16. Thus it
started right out as NEW without ever being explicitly confirmed. The bug is
changed to state NEEDINFO for this reason. To move this bug from NEEDINFO back
to NEW please check if the bug still persists with the 3.5.0 beta1 or beta2 prereleases.
Details on how to test the 3.5.0 beta1 can be found at:
http://wiki.documentfoundation.org/QA/BugHunting_Session_3.5.0.-1

more detail on this bulk operation: http://nabble.documentfoundation.org/RFC-Operation-Spamzilla-tp3607474p3607474.html
Comment 17 Björn Michaelsen 2011-12-23 12:57:17 UTC
An EasyHack should have been checked by developers and thus is confirmed regardless of age. Moving back to NEW from NEEDINFO again. Sorry for the hassle.
Comment 18 Winfried Donkers 2011-12-27 02:58:19 UTC
Created attachment 54840 [details]
modified code so far, still not complete

I did make some progress: the double-function toolbutton shows the last used color and when pressing the left hand button, the color of the selection can be set.
I have done this by using a boolean to check if the popup window (the color palette) has been used; if so, the last used color must be updated.

BUT: to set the color to the correct value, I need to get the last used color. This is stored in mLastColor in SvxFontColorExtToolBoxControl (tbcontrl.cxx). The spot where the color is set, is in ScFormatShell::ExecuteAttr()  (formatsh.cxx).
How do I get this value? I hope any mentor/expert can give me a hint of the function/call to be used. When this is solved, I can complete the code for both background color as font color and submit the patch.

Please help and/or provide comments on the modifications doen so far.
Comment 19 Winfried Donkers 2012-01-19 03:11:23 UTC
Looks like I have solved the problem for the backgroundcolor. I wanted to use the color as stored in the SvxFontColorExtToolBoxControl, but could not get to the control from ScFormatShell::ExecuteAttr() (in formatsh.cxx). Now I have used the same method as with writer: store the last used color in ScTabViewShell. I am not proud of the solution (I'd still rather use the color alreday stored in the control), but it works. I want to do some extra testing and polishing of the code changes before I submit the patch.

The font color is handled differently; I don't see how I can put the needed functionality in ScFormatShell::ExecuteAttr() (in formatsh.cxx) for this control, so I'll leave that control as it is, unless someone has a suggestion that helps me to do it.
Comment 20 Winfried Donkers 2012-01-21 02:43:21 UTC
patch submitted to dev-list
Comment 21 Winfried Donkers 2012-02-01 00:49:15 UTC
Patch has been pushed on master, push to 3.4 and 3.5 branches requested
Comment 22 Larsen 2012-02-01 01:49:13 UTC
@Winfried: Thanks for your work! =)
Comment 23 Uldis Kalniņš 2012-02-01 05:00:14 UTC
Thank You!
Comment 24 Yifan Jiang 2012-02-01 18:09:07 UTC
@Winfried

This's one of my dream feature, thanks for doing that!

Just attempted a bit yesterday and works exactly as what I ever expected :)
Comment 25 Kohei Yoshida 2012-02-15 11:29:39 UTC
*** Bug 46069 has been marked as a duplicate of this bug. ***
Comment 26 Joel Madero 2012-10-15 15:16:41 UTC
*** Bug 38813 has been marked as a duplicate of this bug. ***
Comment 27 Markus Grob 2013-10-08 16:14:16 UTC
*** Bug 57011 has been marked as a duplicate of this bug. ***
Comment 28 Robinson Tryon (qubit) 2015-12-16 00:12:11 UTC
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicUi )
[NinjaEdit]