[GSoC] Refactor ArchiveNode and ArchiveDirNode classes
Closed, ResolvedPublic

Description

Refactor ArchiveNode and ArchiveDirNode classes into one ArchiveEntry class.

mvlabat created this task.May 24 2016, 7:44 PM

This is a good start. Here's my comments:

  1. The ArchiveEntry class should be defined in its own file (within kerfuffle/).
  2. It's really time to let go the map we use for metadata. Just define this metadata as Q_PROPERTYs of ArchiveEntry (use the Archive class as an example!)

Bonus point: it would be super awesome if you could make this new class a nested class of Archive! In other words, instead of calling the new class ArchiveEntry, you would just call it Entry and make it an inner class of Archive. Something like this:

// archiveentry.h
class Archive::Entry : public QObject
{
    Q_OBJECT

    Q_PROPERTY(...)
    Q_PROPERTY(...)
    ...
}

@elvisangelaccio
I wanted to ask you about refactoring some pieces of code..
Now, when we have all metadata contained in Archive::Entry class, we've got the situation, that an Entry instance will be created with plugins. That means, that when we construct an Entry, we fill it with metadata, but all other fields (parent, for instance) are unset at the moment. So, I think, we should provide a method to set parents (not only in constructor) and maybe methods for some other fields if needed.

Also copying metadata has place in the code.
For instance,
archivemodel.cpp: line 511:
EntryMetaData e(entry->metaData());
*entry = new Archive::Entry(parent, e);
There, I think, we should provide some methods for copying metadata between entries. Maybe, a copying constructor or assignment operator for Archive::Entry class would be an easy solution, but as I can see, we shouldn't (or won't be able to) do it because of QObject design.

So I wanted to ensure whether these decisions are OK and I got the code architecture right.

Now, when we have all metadata contained in Archive::Entry class, we've got the situation, that an Entry instance will be created with plugins. That means, that when we construct an Entry, we fill it with metadata, but all other fields (parent, for instance) are unset at the moment. So, I think, we should provide a method to set parents (not only in constructor) and maybe methods for some other fields if needed.

Correct.

Also copying metadata has place in the code.
For instance,
archivemodel.cpp: line 511:
EntryMetaData e(entry->metaData());
*entry = new Archive::Entry(parent, e);
There, I think, we should provide some methods for copying metadata between entries. Maybe, a copying constructor or assignment operator for Archive::Entry class would be an easy solution, but as I can see, we shouldn't (or won't be able to) do it because of QObject design.

So I wanted to ensure whether these decisions are OK and I got the code architecture right.

Yes, QObjects cannot be copied by assignment. Given that also ArchiveModel needs refactoring, you can ignore this problem for now. Just comment every occurrence of "metadata copying" with a // FIXME or // TODO so that you can worry about this later.

mvlabat closed this task as Resolved.Aug 15 2016, 12:44 PM