D19624: [T4538] Collapsable cells

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> commandentry.cpp:283
>  
> +    if (!m_resultItems.isEmpty() || m_collapsedResultsCount != 0) {
> +        if (m_collapsedResultsCount == 0)

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
  }

?

> commandentry.cpp:1180
> +    for (auto* result : m_expression->results())
> +        m_resultItems << ResultItem::create(this, result);
> +    animateSizeChange();

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?

> commandentry.h:100
>      void updateEntry() override;
> -    void updatePrompt();
> +    void updatePrompt(const QString& Postfix = CommandEntry::Prompt);
>      void expressionChangedStatus(Cantor::Expression::Status status);

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

> commandentry.h:137
> +    void collapseResults();
> +    void uncollapseResults();
>      void removeResultItem(int index);

expandResults()

REPOSITORY
  R55 Cantor

REVISION DETAIL
  https://phabricator.kde.org/D19624

To: sirgienko, asemke
Cc: kde-edu, asemke, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190308/5243cd59/attachment-0001.html>