<div dir="ltr"><div class="gmail_default" style="font-family:comic sans ms,sans-serif">Hi everyone,</div><div class="gmail_default"> </div><div class="gmail_default">So here is what happened. </div><div class="gmail_default">My initial call was to add the new variable to the data series (or similar central module), which ensures that any future chart types (post‑2016) can use this variable if they require “calculated” versus “source” data. </div><div class="gmail_default">Now my initial approach was not in the right direction and later Quikee pointed me in the right direction -</div><div class="gmail_default" style="font-family:comic sans ms,sans-serif"><ul><li>Pay close attention to the "You Ain't Gonna Need It" (YAGNI) principle. Don't over-engineer for future chart types. Focus on solving the current problem (histogram support) in a clean and maintainable way.</li><li>No IDL Changes (Unless Necessary): Avoid modifying the UNO IDL files unless you have a very strong reason. IDL changes can have far-reaching consequences.</li><li>Persistence: Quikee said that persistence doesn't need to be accounted for at this point and can be calculated again on the fly as we have the original data.</li></ul><div> Regina pointed out a deeper problem in the approach taken so far. She mentioned that the calculated bin frequencies and bin ranges were set in place of the original data. </div><div>This approach allowed for a convenient way to render the histogram as a bar chart, but it introduced bugs (tdf#164109 and tdf#162240) and made it impossible to export the </div><div><span class="gmail-css-1jxf684 gmail-r-bcqeeo gmail-r-1ttztb7 gmail-r-qvutc0 gmail-r-z32n2g gmail-r-z2wwpe gmail-r-1hkscgl gmail-r-1471scf gmail-r-1aiqnjv gmail-r-1hq4qhi gmail-r-16dba41 gmail-r-ilng1c gmail-r-trst2h gmail-r-1noe1sz gmail-r-njp1lv"><chart:series> element in the same way as other chart types. She referred to a specific part of the code in <span class="gmail-css-1jxf684 gmail-r-bcqeeo gmail-r-1ttztb7 gmail-r-qvutc0 gmail-r-z32n2g gmail-r-z2wwpe gmail-r-1hkscgl gmail-r-1471scf gmail-r-1aiqnjv gmail-r-1hq4qhi gmail-r-16dba41 gmail-r-ilng1c gmail-r-trst2h gmail-r-1noe1sz gmail-r-njp1lv">SchXMLExport.cxx where the export of data series for the current chart type is handled. </div><div>Currently, in the patch being discussed, the <span class="gmail-css-1jxf684 gmail-r-bcqeeo gmail-r-1ttztb7 gmail-r-qvutc0 gmail-r-z32n2g gmail-r-z2wwpe gmail-r-1hkscgl gmail-r-1471scf gmail-r-1aiqnjv gmail-r-1hq4qhi gmail-r-16dba41 gmail-r-ilng1c gmail-r-trst2h gmail-r-1noe1sz gmail-r-njp1lv"><chart:series> element isn’t being exported at all. When it is exported, the new <span class="gmail-css-1jxf684 gmail-r-bcqeeo gmail-r-1ttztb7 gmail-r-qvutc0 gmail-r-z32n2g gmail-r-z2wwpe gmail-r-1hkscgl gmail-r-1471scf gmail-r-1aiqnjv gmail-r-1hq4qhi gmail-r-16dba41 gmail-r-ilng1c gmail-r-trst2h gmail-r-1noe1sz gmail-r-njp1lv"><loext:histogram-configuration> element needs to be a child element </div><div>after all existing child elements, ideally just before the comment <span class="gmail-css-1jxf684 gmail-r-bcqeeo gmail-r-1ttztb7 gmail-r-qvutc0 gmail-r-z32n2g gmail-r-z2wwpe gmail-r-1hkscgl gmail-r-1471scf gmail-r-1aiqnjv gmail-r-1hq4qhi gmail-r-16dba41 gmail-r-ilng1c gmail-r-trst2h gmail-r-1noe1sz gmail-r-njp1lv">'// close series'. </div><div> </div><div>Tomaž has made a patch (PR) <a href="https://gerrit.libreoffice.org/c/core/+/181051">https://gerrit.libreoffice.org/c/core/+/181051</a> to address this, </div><div>and I’ve copied his work so far without making any changes yet <a href="https://gerrit.libreoffice.org/c/core/+/182101">https://gerrit.libreoffice.org/c/core/+/182101</a> .</div><div> </div><div>After his PR, Regina further commented on it -</div><div><a href="https://gerrit.libreoffice.org/c/core/+/181051/comments/ca521ccf_9e2ab59c?tab=comments">https://gerrit.libreoffice.org/c/core/+/181051/comments/ca521ccf_9e2ab59c?tab=comments</a></div><div><h4>UI/UX Issues </h4><ul><li>Problem : <ul><li><code class="gmail-codespan gmail-cursor-pointer">values-y</code> and <code class="gmail-codespan gmail-cursor-pointer">calculated-y</code> share the same UI label (<code class="gmail-codespan gmail-cursor-pointer">STR_DATA_ROLE_Y</code>).</li><li>Switching chart types adds duplicate data series.</li></ul></li><li>Solution :<ul><li>Assign unique labels (e.g., "Original Y Values" vs. "Calculated Frequencies").</li><li>Clear <code class="gmail-codespan gmail-cursor-pointer">calculated-y</code>/<code class="gmail-codespan gmail-cursor-pointer">calculated-x</code> when switching away from histograms.</li></ul></li></ul> </div><div> </div><div>So what does he propose? </div><div>The goal is to ensure that the original data is preserved while also accommodating the calculated values needed for the histogram.</div><div> </div><div>First, let's clarify the problem with the y-values. In a histogram, the y-values represent frequencies, which are calculated from the original data. </div><div>However, the existing code assumes that the "values-y" role always contains the original, raw data. This creates a conflict because for histograms, </div><div>we need to use calculated frequencies for rendering, but we also need to keep the original data intact for other functionalities or potential chart-type changes.</div><div> </div><div> </div><div>To address this, Tomaž introduced a new role called "calculated-y" in his patch. This role is meant to store the calculated frequencies for the histogram, </div><div>while keeping the original data in the "values-y" role. This way, the original data remains untouched, and the calculated values are available for rendering the histogram.</div><div> </div><div>So, one of the tasks is to introduce m_aCalculated_Y in VDataSeries and adjust the code to use m_aCalculated_Y when it's set (i.e., for histograms), and m_aValues_Y otherwise. </div><div>This requires auditing all places where m_aValues_Y is used and deciding whether to use m_aValues_Y or m_aCalculated_Y based on the context.</div><div> </div><div>Additionally, there's a mention of clearing the "calculated-y" role when changing the chart type. </div><div>This makes sense because the calculated values are specific to the histogram and shouldn't persist if the user switches to a different chart type.</div><div> </div><div>Now, regarding the x-axis, there seems to be some confusion. Initially, I thought the issue was with x-values, but Tomaž clarified that the primary issue is with y-values. </div><div>However, in the context of histograms, the x-values represent the bins, which are also calculated from the original data. </div><div>So, similar to the y-values, there might be a need to handle calculated x-values separately to preserve the original data. </div><div> </div><div> </div><div> </div> </div></div> <div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Sat, 1 Feb 2025 at 01:17, Devansh Varshney <<a href="mailto:varshney.devansh614@gmail.com">varshney.devansh614@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-family:comic sans ms,sans-serif" class="gmail_default"></div><div class="gmail_default" style="font-family:comic sans ms,sans-serif">Hi community,</div><div class="gmail_default"> </div><div class="gmail_default">To follow up on the previous mail, I'd like to give you a more detailed update on how we're addressing the histogram data handling issue in the chart2 module, which Tomaž and Regina pointed out.</div><ul><li class="gmail_default">The core problem lies in how histograms handle X-axis values. The existing chart2 architecture assumes a direct mapping between source data and chart X-axis values.</li><li class="gmail_default">However, histograms calculate X-axis values (bin-width) based on the input data, leading to a situation where these calculated values overwrite the original X-values and create issues in many places in the code.</li><li class="gmail_default">We can get the render of the chart while creating it but when we tried to save(for example) the original values were gone by this time hence the problem. </li></ul><div class="gmail_default"> </div><div class="gmail_default">Tomaž also said that he would like to introduce a variable XValuesCalculated which will resolve most of the problem. </div><div class="gmail_default"> </div><div class="gmail_default">Further Analysis -</div><div class="gmail_default"><ul><li> Original X-Values: The original X-values are correctly stored in XLabeledDataSequence.</li><li> Histogram Calculations: The histogram calculations occur within the HistogramDataInterpreter and then the calculated values overwrite the XLabeledDataSequence x values.</li><li> Need for Separation: We need to store and display the histogram-calculated X-values while preserving the original data as many parts of the code depend on original values.</li></ul></div><div class="gmail_default"> </div><div style="font-family:comic sans ms,sans-serif" class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default">Analysis of HistogramChartType.cxx Purpose: This file defines the HistogramChartType class, that is responsible for setting up a histogram chart type. It manages properties specific to histograms,</div><div class="gmail_default">like bin width and range. It also implements the logic to create the data-series and do the calculations. createCalculatedDataSeries() Method: <ul><li>This method retrieves the raw data through the XLabeledDataSequence from m_aDataSeries. This is where the logic modifies the source data (the sequence returned from getValues()).</li><li>It then uses HistogramCalculator to perform calculations and generate the data.</li><li>Finally, it creates a new HistogramDataSequence to store calculated values and then adds that to m_aDataSeries.</li></ul> Interaction with XLabeledDataSequence: <ul><li> It gets the original data using the getDataSequences2() method of DataSeries.</li><li> It gets the original x values by using getValues() method of the XLabeledDataSequence which returns an XDataSequence and then uses the getData() method of the XDataSequence to retrieve the x values in a sequence of uno::Any.</li></ul></div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default">Analysis of HistogramCalculator.cxx Purpose: This class performs the core histogram calculations using the raw data that is provided to computeBinFrequencyHistogram() method. Bin Calculation Logic: <ul><li> The class calculates the number of bins, bin width, and bin ranges based on the input data.</li><li> It computes bin ranges and their frequencies.</li><li> The calculated values are stored internally in maBinRanges and maBinFrequencies.</li></ul></div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default">Understanding HistogramDataSequence.cxx Data Storage: <ul><li> The primary data storage is the mxValues member, which is a uno::Sequence<double>. This sequence stores the calculated bin values for the histogram.</li><li> There is also mxLabels which stores labels but we are not concerned with it.</li></ul> Interface Implementations: <ul><li> HistogramDataSequence implements several UNO interfaces including XNumericalDataSequence, XTextualDataSequence, XDataSequence.</li><li> getNumericalData(): Returns the mxValues as a uno::Sequence<double>.</li><li> getTextualData(): Returns an empty uno::Sequence<OUString>.</li><li> getData(): Returns the mxValues as a uno::Sequence<uno::Any>.</li></ul></div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default"><div class="gmail_default">Proposed Solution:</div></div><div class="gmail_default">To address these issues, this is how I am thinking of making the changes: * Add XValuesCalculated Property(As said by Tomaž) : <ul><li> Add an [optional, attribute] sequence<double> XValuesCalculated; to the DataSeries service definition (offapi/com/sun/star/chart2/DataSeries.idl).</li><li> Implement the corresponding storage, getter, and setter methods in DataSeries.hxx and DataSeries.cxx. (Add storage for m_aXValuesCalculated)</li><li> Add the corresponding property support in OPropertySet.hxx and OPropertySet.cxx?</li></ul></div><div class="gmail_default"> Why add it to DataSeries? -> If my understanding is correct, XValuesCalculated is a property associated with a DataSeries and is not a new data structure by itself, so we do not need to create a new IDL file for it.</div><div class="gmail_default"><div class="gmail_default"> No Boolean Flag Needed: -> we do not need an explicit boolean flag. I am using .hasElements() and why Length Check is Sufficient:<ul><li> Presence = Calculated Values: The presence of elements within the XValuesCalculated sequence is sufficient to determine if calculated X-values exist.</li><li> Empty Sequence = No Calculated Values: An empty sequence, which tells us that no values have been calculated. This is very convenient because UNO Sequences can be empty.</li><li> No Redundancy: Using a separate boolean flag would add redundancy. </li></ul></div></div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default"> * Modify Histogram Logic: <ul><li> Update HistogramDataInterpreter.cxx to perform calculations and store calculated bin values in the XValuesCalculated property of the corresponding DataSeries object. This will not touch the original values in the XLabeledDataSequence.</li><li> And the Histogram View utilises the BarChart view for the final rendering.</li></ul> * Modify Rendering Logic: <ul><li> Modify chart renderers (such as VSeriesPlotter.cxx ?) to prioritize the XValuesCalculated property when available for plotting. When the XValuesCalculated is not available then it will use the original values of XLabeledDataSequence.</li></ul> * Adjust Axis Scaling: <ul><li> Modify the code to use the XValuesCalculated property, when available, for axis scaling range calculation.</li></ul> * Preserve Original Data in UI: <ul><li> Ensure that UI components always display original data fetched from XLabeledDataSequence with the role of "values-x".</li><li> Since users expect the UI tables to reflect the underlying data they have provided. If we show calculated bin values in tables/cells, it will be confusing and wrong.</li></ul> * Comprehensive Testing: <ul><li> Create tests to ensure that the changes work and do not cause a regression to ensure proper functionality and maintain backward compatibility.</li><li> We will also make sure the undo and redo are working properly.</li></ul> * Documentation: <ul><li> Document all the changes to reflect the new property and its usage.</li></ul></div><div class="gmail_default"> </div> <div class="gmail_default">Key Benefits: <ol><li> Correct Histogram Display: Histograms will correctly use calculated bin values for the X-axis.</li><li> Preserved Original Data: Original source data will be maintained for other charts and user interactions.</li><li> Minimal Impact: This change does not affect other chart types, it ensures backward compatibility, and it only impacts histogram and renderer-specific logic.</li><li> Clear Separation: Separates the concern of raw data and calculated data.</li><li> Improved Architecture: Addresses the underlying issue of assumptions embedded in the chart(chart2) module.</li></ol></div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default">Is the below file related? </div><div class="gmail_default"><a href="https://github.com/LibreOffice/core/blob/b0a4afc58e1c9434e56ddb96c41f4ebe5985ed0a/chart2/source/controller/dialogs/DataBrowser.cxx#L4" style="font-family:tahoma,sans-serif" target="_blank">chart2/source/controller/dialogs/DataBrowser.cxx</a> </div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default"> </div><div class="gmail_default" style="font-family:comic sans ms,sans-serif">Do we also have to modify getValues() in XLabeledDataSequence to return calculated values or original values based on the presence of XValuesCalculated property and chart type?</div><div class="gmail_default" style="font-family:comic sans ms,sans-serif"> </div><div class="gmail_default" style="font-family:comic sans ms,sans-serif"></div><div class="gmail_default" style="font-family:comic sans ms,sans-serif">If other places need changes or I if have still missed files (please let me know)</div><div class="gmail_default" style="font-family:comic sans ms,sans-serif">and the changes in the offapi/com/sun/star/chart2/DataSeries.idl are correct?</div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 23 Jan 2025 at 18:59, Regina Henschel (via Code Review) <<a href="mailto:gerrit@gerrit.libreoffice.org" target="_blank">gerrit@gerrit.libreoffice.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Attention is currently required from: Devansh Varshney, Tomaž Vajngerl. Regina Henschel has posted comments on this change by Devansh Varshney. ( <a href="https://gerrit.libreoffice.org/c/core/+/177364?usp=email" rel="noreferrer" target="_blank">https://gerrit.libreoffice.org/c/core/+/177364?usp=email</a> ) Change subject: xmloff: Histogram Chart ODF import and export support ...................................................................... Patch Set 26: (2 comments) Patchset: PS26: There is a deeper problem. You have set the calculated binFrequencies and binRanges in place of the original data. That has give you a convenient way for getting a rendering as bar chart. Not only does this cause bugs tdf#164109 and tdf#162240, but now you can't export the <chart:series> element the same way as it is done for other chart types. I'm referring to the way that can be seen after comment '// export dataseries for current chart-type' in SchXMLExport.cxx. Currently in your patch, the <chart:series> element is not exported at all. When it will be exported, the new <loext:histogram-configuration> element has to become a child element after all the existing child elements. If the export looks similar to the existing chart types, it would be just before the comment '// close series'. I suggest to discuss the underlying problem with Tomaž Vajngerl. I don't see an easy solution and would have to try a lot. Until this is resolved, it doesn't make sense to make progress on the export. File schema/libreoffice/OpenDocument-v1.4+libreoffice-schema.rng: <a href="https://gerrit.libreoffice.org/c/core/+/177364/comment/2e8b89d8_b939b3f4?usp=email" rel="noreferrer" target="_blank">https://gerrit.libreoffice.org/c/core/+/177364/comment/2e8b89d8_b939b3f4?usp=email</a> : PS26, Line 2149: <rng:optional> : <rng:ref name="loext-histogram-configuration"/> : </rng:optional> > I mean the `loext:histogram-configuration` is placed under `chart:series` is this the correct way? […] The position is correct. -- To view, visit <a href="https://gerrit.libreoffice.org/c/core/+/177364?usp=email" rel="noreferrer" target="_blank">https://gerrit.libreoffice.org/c/core/+/177364?usp=email</a> To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.libreoffice.org/settings?usp=email" rel="noreferrer" target="_blank">https://gerrit.libreoffice.org/settings?usp=email</a> Gerrit-MessageType: comment Gerrit-Project: core Gerrit-Branch: master Gerrit-Change-Id: Ibda198c1a3e4a7b53e3cf590c33cc9adb558be74 Gerrit-Change-Number: 177364 Gerrit-PatchSet: 26 Gerrit-Owner: Devansh Varshney <<a href="mailto:varshney.devansh614@gmail.com" target="_blank">varshney.devansh614@gmail.com</a>> Gerrit-Reviewer: Jenkins Gerrit-Reviewer: Tomaž Vajngerl <<a href="mailto:quikee@gmail.com" target="_blank">quikee@gmail.com</a>> Gerrit-CC: Regina Henschel <<a href="mailto:rb.henschel@t-online.de" target="_blank">rb.henschel@t-online.de</a>> Gerrit-Attention: Devansh Varshney <<a href="mailto:varshney.devansh614@gmail.com" target="_blank">varshney.devansh614@gmail.com</a>> Gerrit-Attention: Tomaž Vajngerl <<a href="mailto:quikee@gmail.com" target="_blank">quikee@gmail.com</a>> Gerrit-Comment-Date: Thu, 23 Jan 2025 13:29:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Devansh Varshney <<a href="mailto:varshney.devansh614@gmail.com" target="_blank">varshney.devansh614@gmail.com</a>> </blockquote></div><div> </div> -- <div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,</div><div>Devansh </div></div></div> </blockquote></div><div> </div> -- <div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,</div><div>Devansh </div></div></div>