[T4538] Collapsable cells
ClosedPublic

Authored by sirgienko on Mar 8 2019, 7:46 PM.

Details

Summary

Add feature to hide and show results of command entry via populate menu

Diff Detail

Repository
R55 Cantor
Branch
collapsable-cells
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9355
Build 9373: arc lint + arc unit
sirgienko created this revision.Mar 8 2019, 7:46 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptMar 8 2019, 7:46 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
sirgienko requested review of this revision.Mar 8 2019, 7:46 PM
asemke added inline comments.Mar 8 2019, 9:21 PM
src/commandentry.cpp
283 ↗(On Diff #53474)

do you really need this new variable m_collapsedResultCount here? You're clearing m_resultsItems anyway. Why not to check like

if (m_expression->results().size())
{
    if (m_resultItems.size())
        //result(s) available and result item(s) shown -> allow to hide
    else
        //result(s) available but result item(s) hidden  -> allow to show
}

?

1180 ↗(On Diff #53474)

won't we run into problems with the performance for rendered image results for example? Do we really need to remove items and create them again? Why not to simple hide/show them without deleting them?

src/commandentry.h
100 ↗(On Diff #53474)

lower case for variables. Maybe use 'promt' as the name of the variable?

137 ↗(On Diff #53474)

expandResults()

sirgienko updated this revision to Diff 53480.Mar 8 2019, 9:35 PM

Minor improvments

sirgienko marked 2 inline comments as done.Mar 8 2019, 9:43 PM
sirgienko added inline comments.
src/commandentry.cpp
283 ↗(On Diff #53474)

I think, we really need this variable in updateEntry. updateEntry called, when we redraw worksheet or when we have new result. So, m_collapsedResultCount, stored count of collapsed results, needed to distinguish the first situation from the second.

1180 ↗(On Diff #53474)

I agree, this is not very effective. But, I can't found another solution. I have tried show/hide; disable/enable, but results item in this cases don't free their places, so we have empty space between command entries. So, I open to suggestions

sirgienko updated this revision to Diff 53538.Mar 9 2019, 7:52 PM

Instead of removing results, starts hiding them

sirgienko marked 4 inline comments as done.Mar 9 2019, 7:53 PM
asemke accepted this revision.Mar 9 2019, 7:59 PM
This revision is now accepted and ready to land.Mar 9 2019, 7:59 PM
This revision was automatically updated to reflect the committed changes.