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

Authored by sirgienko on Apr 5 2018, 2:46 PM.

Details

Summary

Before, if you save worksheet and load it, you have see, that after each line have added empty entry. It happens, because command entry after placing result checks existence next entry. If entry don't exist, the command entry add new empty command entry, so worksheet always have empty entry on the end of the worksheet. But when we load entries from file, we haven't empty entry on the end, because we place entries one by one. So each time, when we place command from file, the command after placing result (from file too) add empty entry. It fix this problem by adding check before adding empty entry, that we don't load entries from file.

Test Plan
  1. Write multiline worksheet
  2. Save it
  3. Close it
  4. Open it
  5. Checks, that there aren't unnesesary empty entries.

Diff Detail

Repository
R55 Cantor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sirgienko created this revision.Apr 5 2018, 2:46 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptApr 5 2018, 2:46 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript
sirgienko requested review of this revision.Apr 5 2018, 2:46 PM
asemke added a comment.Apr 5 2018, 3:12 PM

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().

sirgienko added a comment.EditedApr 5 2018, 3:15 PM

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().

asemke added a comment.Apr 5 2018, 3:24 PM

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) is of no benefit here and blocks my proposal for a simple fix for the problem you're addressing here.

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.

asemke added a comment.Apr 5 2018, 4:21 PM

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.

The compiler will most probably inline those calls, yes.

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.

This is a very valid argument.

asemke accepted this revision.Apr 5 2018, 4:21 PM
This revision is now accepted and ready to land.Apr 5 2018, 4:21 PM
This revision was automatically updated to reflect the committed changes.