Apache OpenOffice (AOO) Bugzilla – Full Text Issue Listing |
Summary: | Display of hidden cells | ||
---|---|---|---|
Product: | General | Reporter: | IngridvdM |
Component: | chart | Assignee: | kla <thomas.klarhoefer> |
Status: | CLOSED FIXED | QA Contact: | issues@graphics <issues> |
Severity: | Trivial | ||
Priority: | P3 | CC: | amy2008, issues, kyoshida, yoshimit |
Version: | 3.3.0 or older (OOo) | Keywords: | ms_interoperability, usability |
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
URL: | https://bugzilla.novell.com/show_bug.cgi?id=408424 | ||
Issue Type: | FEATURE | Latest Confirmation in: | --- |
Developer Difficulty: | --- | ||
Attachments: |
Description
IngridvdM
2008-07-11 15:03:30 UTC
Created attachment 55072 [details]
example scenario
It seems that the behavior for this scenario has changed several times in the past and always there were users that were not happy with the current behavior so it has become necessary to offer an option to choose. *** Issue 80626 has been marked as a duplicate of this issue. *** *** Issue 52800 has been marked as a duplicate of this issue. *** In the user interface I could imagine a simple check box 'Display values from hidden cells' or similar. Two possible places for the check box come to my mind: 1) On the 'Data Range' tab page - just below 'First column as label' maybe. 2) 'Options' tab page within the series properties dialog. Issue 65549 in CWS chart22 does introduce the vaguely related setting 'Plot missing values' exactly on the series options page. So are there any user opinions about where to place the check box? @kohei, yes for the file format we would need only a simple boolean. I think it would be elegant to define the new attribute in parallel to the somehow related attribute 'chart:treat-empty-cells' - having it in the style at the plot area. Here is a first try for the proposal: ---- The chart:display-hidden-cells attribute specifies whether values from hidden cells are displayed within the chart or not. This attribute can be used within styles that are applied to a <chart:plot-area> element. The chart:treat-empty-cells attribute may be used with the following element: <style:chart-properties> <define name="style-chart-properties-attlist" combine="interleave"> <optional> <attribute name="chart:display-hidden-cells"> <ref name="boolean"/> </attribute> </optional> </define> ---- I am not sure about a default. In the past the behavior has changed again and again. Kohei, as you are on the TC list, would you mind to propose this new attribute? With luck we can have it for ODF1.2 maybe. That would be cool. Oh there is one wrong 'treat-empty-cells' in the proposed proposal text. That should be replaced with 'display-hidden-cells', sorry. Yeah I can work on proposing that to the TC. It should be fairly simple since it's just one boolean, but you never know.... I'll keep you updated with the progress. @kohei, cool thanks! Some hints for the implementation: There is already a property that is very similar implemented to what we need now. It is the check box 'show bars side by side' on the options page in the series properties dialog for bar and column charts. So it can serve as example. In the UNO API we need a new property at the diagram maybe 'DisplayHiddenCells'. The stable ::com::sun::star::chart API is implemented in chart2/source/controller/chartapiwrapper. Add the new property to the DiagramWrapper class (good example is 'GroupBarsPerAxis'). The inner chart model uses not the old stable API but a new ::com::sun::star::chart2 API instead. There we need to add the same new property at the 'inner' diagram class ( look at chart2/source/model/main/Diagram.cxx). In xmloff we need a new token 'display-hidden-cells'. And the new token must be mapped to the new UNO property. Grep for XML_GROUP_BARS_PER_AXIS in xmloff for an example. Thus the new property will be written from and to the file. Now the new property must be transported to the view for display purposes. Search for 'GroupBarsPerAxis' within chart2/source/view/ChartView.cxx to have an example. The view does set the property from the diagram model to a view class 'VDataSeries'. We will need a new method SetDisplayHiddenCells(). VDataSeries is the container for the differenet VDataSequence where we finally need the property (you found the boolean bDisplayHiddenCells there already :-) ). Finally the dialog related things are implemented in chart2/ source/controller/dialogs and chart2/ source/controller/itemsetwrapper. All the ItemConverters within itemsetwrapper serve the purpose to transport the UNO API properties from the model object into SfxItemSets which is a typical input format for several dialogs and tab pages. And vice versa of course. I hope the hints are useful for you. Have fun! Ok. The ODF TC seems to be in favor of the change we have proposed.[1] There are some minor suggestions for change, but none are blockers. [1] http://wiki.oasis-open.org/office/New_Chart_Option_on_Hidden_Cells *** Issue 28932 has been marked as a duplicate of this issue. *** started. I've pretty much finished all tasks except for the ODF import/export & check box in the UI. I'll work on those once we get approval on the ODF spec change from the TC. As for the location of the check box (sorry I didn't see that you asked for my opinion earlier), I would like the check box to stay close to the check box for the "plot missing values" option. These two options IMO should be placed side-by-side if possible. Yes, that sounds like a good idea. The affected tab page class is SchOptionTabPage in files tp_SeriesToAxis.src -.cxx -hxx. The "plot missing values" option was introduced there in the 3.1 code line starting with dev300m29. BTW the core work is already done & ODF import/export completed. I will start the UI work once I get started on upstreaming the code (since the 3.0 codeline that I currently work on in ooo-build doesn't have the treat empty cells addition). just tried to create a cws for this. But 'cws create DEV300 charthiddencells' created a new svn branch but failed to connect to the EIS. I have manually created an entry for that branch via EIS web interface, but it's still incomplete. So, at the moment I have an out-of-sync cws branch that needs to be repaired by someone.... :-( I've filed Issue 95828 for the cws create problem. Created attachment 58306 [details]
new tab page for plotting options
@iha: I've re-designed the Options tab page and attached the new layout. What do you think of this layout? I had to rename the fixed line text from 'Plot missing values' to 'Plot options' so that I could squeeze the include-hidden-cells check box there. I've gone through a couple of different layouts, but I like this one best. Let me know what you think. Created attachment 58307 [details]
options tab page for pie chart
@kohei: Thanks! I think the layout could do. Maybe I would choose slightly different strings: 'Plot missing values' instead of 'Missing values' and 'Plot values from hidden cells' instead of 'Include hidden cells', but I am not a native speaker. I'll attach an according screen shot. Here is the link to the specification from the 'Missing value' feature: http://specs.openoffice.org/chart/MissingValues.odt I think it would be suitable to simply extend this specification to include the new check box. The spec needs to be changed anyhow now. Have a great day! Created attachment 58323 [details]
screenshot - different wording
@iha: thanks! I think your wording sounds better. I'll make those changes & the spec (hopefully I still remember how to access it...) P.S. I just got back from our long holidays. @kohei, the spec is located in module specs/www/chart/MissingValues.odt. It should be accessible via cvs still. P.S.: I hope you had nice holidays! I will start my longer vacation the day after tomorrow :-) Ok. Just updated the spec. @iha: do you think I can assign tk as the QA rep for koheichart01? This is now implemented in koheichart01 cws. @kohei,
> do you think I can assign tk as the QA rep for koheichart01?
Do you mean Thomas Klarhöfer (kla)? Yes, you can. But he is already completely
busy for 3.1 feature freeze. So if target 3.2 is ok for you then set Thomas as
QA rep!
Why are formulas set dirty when the chart starts listening (markRangeDirty)? That looks like a strange hack. @nn: that's not a hack. Under some circumstances value changes do not trigger formula broadcasters to send out value changed broadcast to their listeners, especially when the source ranges are outside the visible area and consist of formula cells. Marking them dirty is the only way to ensure that the broadcast occurs on value change. What are these circumstances? If formula broadcasts don't work, other areas beside charts are affected, and that should be fixed. If it's just about the charts, then the fix should be limited to charts, and not mess around with formulas. This just doesn't seem right. Setting formulas dirty is hardly "messing around with formulas" since no tokens are modified. Here are the circumstances. When source ranges are formula cells outside the visible area, that listens on value cells whose values get changed by the API. When the values of the listened cells change via API, those formula cells are not re-calculated until they become visible, which causes the chart to not get updated. Marking them dirty once when the chart gets activated solves this issue. BTW delaying re-calculation until they become visible is "correct" in its own right. That only becomes problem when an object such as chart depends on their values being up-to-date even when they are not visible. So, to me marking them dirty to force re-calculation on the next value change upon chart creation seems logical to me. Formula cells do of course re-broadcast changes even if they are not visible (there might be visible cells depending on them). A formula cell does not propagate the broadcast if it was already dirty, on the assumption that if the previous value wasn't used, all formulas that depend on the value are also still dirty from the previous broadcast. If you run into this because the cell values aren't read, what you need is the opposite of setting them dirty. Calculation (and with it, clearing the dirty flag) can be forced by reading the result value. ScFormulaCell::SetDirty does more than setting the flag, but you're relying on quite a chain of effects here, which seems too fragile. Ok. I've removed the controversial markRangeDirty method for now. That means that my test case that the method fixes will be back to a broken state, but at least it won't mess up the existing formula handling because of this code change. I will look into that formula issue separately. Since it's hard to reproduce it in a small test case document, and the only reproducing document I have is very large & impossible to debug with it, I'd like to tackle is as a separate task. Jon (Pryor) did a QA on this, and found a bug with the tool tips. The tool tips in a chart act as if all data points are shown. I need to look into this to fix it. Just fixed the tooltip issue. The tooltips inside chart objects now display proper data point values even when some of them are hidden. This is once again fixed in koheichart01 cws. Reopening it as we uncovered another regression. It is actually filed in Issue 97738, so the detail of the regression is described there. In short, the x-axis (category axis) fails to display its values if they are numeric, which also includes dates. I'm looking in this here: https://bugzilla.novell.com/show_bug.cgi?id=463199 just fixed the x-axis issue. Re-opening this again. The tooltip issue is not entirely fixed. The value is displayed correctly, but the category name isn't. I need to fix this. Luckily it was not so hard to fix. Fixed again. @kohei, I found that the current version does break charts with categories from writer tables. The properties 'IsHidden' and 'HiddenValues' are optional and not supported by writer so far. Load the attached document ChartFromWriterTable.odt, double click the chart and you will see how the diagram vanishes, what it should not. Created attachment 60139 [details]
example chart with data from a writer table
Another problem is with charts that do not have a range for categories. For those charts there should be automatically generated some strings, but they are missing now. Load the attached document ChartWithoutCategoryRange.ods and double click on the chart. You will see how the categories vanish what they should not. Created attachment 60140 [details]
chart without a range for categories
@kohei, I have fixed both problems already locally and could commit the changes directly to the CWS. Would that be OK for you? @iha: yup, that's fine. Please commit away. :-) BTW, both problems sound familiar. Those were something we had encountered in ooo-build and I thought I had already fixed that in koheichart01 as well... :-/ Anyway, thanks for the fix. @iha: BTW I'm going to resync that cws once koheiformula02 gets integrated. koheiformula02 modified chart2uno.[hc]xx quite a lot, so I have no doubt that its integration will clash with the change in koheichart01. I have commited fixes for both above described problems to CWS koheichart01. @kohei, are there more bugs in CWS koheichart01 that are already known? I am asking because I am willing to have a deeper look into the chart changes on this CWS and have the time to do so at the moment. But if it is an outdated version I'd better wait until you have brought the known fixes into it maybe. What do you think does it make sense to look at further at the moment? I have seen for example that the legend entries for 'hidden' series do not hide. @iha: no, it's not outdated. I have made every effort to sync this cws with ooo-build's. Those fixes that I mentioned were already in koheichart01 (as I just checked) but I guess it didn't cover the problems you just found. I was not aware of the legend entry breakage, so most likely it's still broken. And I'm not aware of any other bugs that may be in koheichart01. So, please go ahead and fix any problems you find in that cws. :-) Thanks! Ok fine! :-) So just drop me a note before you resync thus I stop commiting then ;-) I found and fixed a layout problem with the new controls in case of stock chart. Fixed another problem: Load the attached document 'HiddenSeries.ods' and double click on the two charts. There should not be a space between the two series. And there should not be a legend entry for Column E because it is hidden and should not be plotted. Created attachment 60240 [details]
example with wrong space and wrong legend entry
Yup, verified both problems (and the fixes). Thanks! Fixed another problem within the range highlighting: Load the attached file 'HiddenSeries.ods'. Within the column chart select the second data point of the first(blue) series. What should happen is that cell D9 gets highlighted. I will look for more tomorrow. This feature spreads quite ugly over the chart code as it affects potentially all places where ranges are used. The feature 'Plot missing values' is broken by this CWS. The data comes already wrong from the calc. m_aDataArray in ScChart2DataSequence contains wrong Items. mfValue is zero where it should be NAN and mbIsValue is true where it should be false. I'll attach a document 'MissingValue.ods' to illustrate the problem. @kohei, as this is calc code could you please have a look what is going wrong there? Thanks! Created attachment 60289 [details]
double click the chart to see the problem with missing values
I fixed another problem with that feature. The radio button 'Leave Gap' could not be clicked anymore. When arranging controls side by side one must be careful that the controls do not overlap otherwise the underlying control is not reachable anymore. Those controls are sized and placed dynamically now. hmm... That UI was working before though. I remember I had to work on that to dynamically relocate the controls... Ok. The missing value treatment issue is now fixed. The fix was very easy; just changing the default values of Item's members to non-value was enough to fix it. @kohei, thanks for the fix. I thought also that changing the constructor defaults might be sufficient. I found that the same error is contained in CWS koheiformula02. I am going to transport the fix therein. Regarding the UI - it was not working before my first checkins to this CWS. I have that version installed still. The relocation was dynamically in y direction already that is right but not in size and x direction. Well it is a pain to not have a layout engine - sigh. A little help here comes with the VCL debug options. You can activate them with Ctrl-Shift-Alt-D. Check the check box 'Dialog' and the current size of the controls will be displayed as green rectangle if you open a new dialog. I found and fixed some more problems: When copying a chart with hidden and not plotted data and pasting it into writer or impress the hidden data is not hidden anymore. The nasty thing here is that the error is not visible immediately - at least in impress. If you double click the chart or start your presentation you will see that your copy looks different than the original. Another problem was with the trendline equation that is displayed in the status bar. Open the attached document TrendlineEquation.ods and select the trend line. Look at the text in the status bar. The formula should read f(x)=x and not f(x)=0.6571x+0.1905. Created attachment 60555 [details]
example for status bar text of selected trenline
It makes no sense to fix another place and another place and another place where values from ranges are used, so I redesigned the interface for this feature. The chart now controls the data provider and the data sequences which got a new property 'IncludeHiddenCells'. If that property is set to false the hidden values are not propagated to the chart. @iha: Yup, that design change makes sense. Thanks for keeping me in the loop. Some more changes: I changed the default. Now newly created charts do not display values from hidden cells. This is what is expected for user scenarios with charts from filtered data (see issue 81209). The new check box now is only visible in case the data provider does support this feature. So currently the check box is only visible if the chart does retrieve its data from a spreadsheet. I updated the specification accordingly and send it to the other i-Team member for review: http://specs.openoffice.org/chart/MissingValues.odt There was also a bug with undo/redo after I changed the interface design. And finally I removed some code that was originally introduced for this feature but finally not used. I hope thats it now - but waiting for review feedback. Feedback from i-Team received: The text from the new check box should better be 'Include values from hidden cells' instead of 'Plot values from hidden cells' as the latter text might be misinterpreted that 'only' values from hidden cells are plotted. ->I updated specification and implementation accordingly. I created issue 100231 to adapt the automatic test scripts for this feature. This needs to be done within the same CWS as the feature implementation. I found that after the interface redesign the import from xls did not work anymore. I fixed this now. I also added the import from xlsx. @kla, please verify in CWS koheichart01. Created attachment 62346 [details]
TCS Display_of_hidden_cells.htm
Issue verified, TCS written *** Issue 102369 has been marked as a duplicate of this issue. *** Verified in DEV300m54 on Ubuntu. Closing |