SRP violation.

[Thejuampi]

https://github.com/junrrein/pdfslicer/blob/8c74b34d44ef85cd206dbed5d9ad7dd715e36977/src/application/application.cpp#L74

AppWindow should be responsible only of displaying data. You could have an DocumentOpener that knows only how to open a document. This implies having an interface that could be tested.

DocumentOpener::open(File aDocumentFile)

In the same way, there could be a DocumentSaver with a save(aDocument)

[junrrein]

This issue is more complex than what can be seen at first hand.

In the first place, a Gtk::ApplicationWindow already has more than one responsibility (as the inheritance graph hints):

• displaying data, and
• managing application-wide actions (actions that can be activated with both a button and a keyboard shortcut, for example).

With that being said, let's focus on the particular issue you raised.

If you look at the implementation of AppWindow::OpenDocument you will see that only the first line is involved in opening the document:

https://github.com/junrrein/pdfslicer/blob/8c74b34d44ef85cd206dbed5d9ad7dd715e36977/src/ui/appwindow.cpp#L49

Everything else deals with displaying the newly loaded Document. So the method is badly named.

For now, I propose taking that first line out of the method, rename the method to setDocument, and have a Document be the parameter to the method (instead of a file). Edit: Done in 3aa1c888e65a78bc1a4d5787439a67d45708a858

I'll talk about the SRP violation in the next comment.

[junrrein]

The interface of Document is the following:

public:
    Document(const std::string& filePath);
    ~Document();

    void saveDocument(const Glib::RefPtr<Gio::File>& destinationFile);
    void removePage(int pageNumber);
    void removePages(const std::vector<unsigned int>& positions);
    void removePageRange(int first, int last);
    void rotatePagesRight(const std::vector<unsigned int>& pageNumbers);
    void rotatePagesLeft(const std::vector<unsigned int>& pageNumbers);
    void undoCommand() { m_commandManager.undo(); }
    void redoCommand() { m_commandManager.redo(); }

    bool canUndo() const { return m_commandManager.canUndo(); }
    bool canRedo() const { return m_commandManager.canRedo(); }
    const Glib::RefPtr<Gio::ListStore<Page>>& pages() const { return m_pages; }

    sigc::signal<void>& commandExecuted() { return m_commandManager.commandExecuted; }
    sigc::signal<void, std::vector<unsigned int>> pagesRotated;

We can separate the responsibilites into three parts:

• Opening the document, done by the constructor.
• Saving the document (saveDocument).
• Altering the document (removing/rotating pages, undoing actions) and offering event notifications for these alterations.

If we can agree that these are the responsibilities, then the class should be separated into three:

• DocumentLoader
• DocumentSaver
• Document (which would deal with document alteration and notifications of these alterations)

I'm not sure about how to technically go about doing this. This is how I've thought about it:

• For DocumentLoader: Make the constructor of Document private, declaring DocumentLoader as a friend class, so that DocumentLoader is the only one able to create an instance of a Document.
• For DocumentSaver: either declare this as a friend class of Document, or expose Document.m_sourcePath in the public interface (we need the source path in order to save the document).

What do you think?

[junrrein]

We now have separate DocumentSaver and PageRenderer classes.

I'm not liking the idea of a DocumentLoader class since it will be too tightly coupled with the private members of Document.