D11962: Fix problems with unnesesary unsaved empty entries when load worksheet from file

[Nikita Sirgienko]

sirgienko added a comment.


  In D11962#240607 <https://phabricator.kde.org/D11962#240607>, @asemke wrote:
  
  > In D11962#240606 <https://phabricator.kde.org/D11962#240606>, @sirgienko wrote:
  >
  > > In D11962#240605 <https://phabricator.kde.org/D11962#240605>, @asemke wrote:
  > >
  > > > A simple
  > > >
  > > >   if (m_isLoadingFromFile)
  > > >       return;
  > > >
  > > >
  > > > in the beginning of Worksheet::appendCommandEntry() should do the same, right? With this we don't need any changes in worksheetentry.cpp and any Worksheet::isLoadingFromFile().
  > >
  > >
  > > In `Worksheet::load(QIODevice* device)` we also call `appendCommandEntry()`.
  >
  >
  > I'd rather call appendEntry(CommandEntry::Type), etc. directly in Worksheet::load(QIODevice*) instead of going via all those append*Entry() functions - this additional indirection (one more function call)
  
  
  I am not sure, that compiler don't optimize it in inline call.
  
  > is of no benefit here and blocks my proposal for a simple fix for the problem you're addressing here.
  
  I think your solution simplier, but hachier too. I mean, in `appendCommandEntry` logic of ignoring is obvicious, but from interface point of view, we sometimes ignore adding command (and only commands) entries (but don't ignore inserting or inserting before). And it's no good, I think, if caller don't know, what entry don't added, because we return `nullptr` instead valid `WorksheetEntry*`, and it cause probleams with dereference of nullptr pointer.

REPOSITORY
  R55 Cantor

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

To: sirgienko, #cantor, asemke
Cc: #cantor, #kde_edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180405/0c77ab63/attachment.html>