Workspace sidebar #5189

MatteoGalletta · 2023-10-01T10:21:33Z

Added Workspace sidebar (by default it is hidden, open it from "View > Show Workspaces").

Tries to close issue #799 inspiring from Notepad++ workspace behaviour.

Features:
-Add folders to the workspace from the File menu "Add folder to Workspace" or using the button on the sidebar
-Remove all folders from the workspace from the File Menu "Clear and close workspace" or using the button on the sidebar (the button doesn't close the workspace tho, it just clears it)
-Show/Hide the workspace from View > Show Workspace
-Resize the workspace (the width gets stored in the settings)
-The workspace is just one and it's saved in ~/.config/xournalpp/workspace.txt and is persistent between app executions.
-Immediatly open the files contained in the folders added to the workspace by double-clicking on them. The files are filtered by extension and the ones visible are the ones that Xournal++ can open (which are PDF, xopp, xoj, xopt, moj).

Known Issues:
-F8 shortcut doesn't work in most of the situations (e.g. if you start drawing)
-There's the following warning (I tried to remove the property but then the GtkPaned behaviour changes): "GtkBox does not have a child property called shrink"

Next Steps (I'd like to follow):
-Add icons (for folders and different extensions files) - currently files and empty folders look the same
-Improve UI colors (currently the TreeView and the label background are slightly different)
-Allow to remove a single folder
-Implement other right click functionalities (such as rename/delete file, copy path, etc)

I also fixed a bug (maybe I should have made a separate pull request but it was just an if statement) in the sidebar, which could be replicated by doing such steps:
1. Resize the sidebar (and remember the new width).
2. Hide the sidebar (F12)
3. Close the app, open the app and close it again.
4. Open the app and show the sidebar (F12).
5. You can notice that the sidebar width hasn't been saved as it should. Now it's fixed.

I tested the changes only on (Manjaro) Linux.

I'm new to contributing, I can see that the main.glade file changes list is huge. I tabbed everything to add a wrapper container but looks like git doesn't understand it?

rolandlo · 2023-10-02T12:09:56Z

Thanks for working on this feature. It already looks quite useful even though only basic functionality has been implemented so far. Some quick feedback:

Currently compilation fails on MS Windows. I think Workspace::fillTreeFromFolderPath should accept a fs::path, not a std::string as folderPath.

D:/a/1/s/src/core/gui/Workspace.cpp: In member function 'virtual void Workspace::addFolder(std::filesystem::__cxx11::path)':
D:/a/1/s/src/core/gui/Workspace.cpp:110:42: error: cannot convert 'std::filesystem::__cxx11::path' to 'std::string' {aka 'std::__cxx11::basic_string<char>'}
  110 |     fillTreeFromFolderPath(store, &iter, folderPath);
      |                                          ^~~~~~~~~~
      |                                          |
      |                                          std::filesystem::__cxx11::path
D:/a/1/s/src/core/gui/Workspace.cpp:52:94: note:   initializing argument 3 of 'void Workspace::fillTreeFromFolderPath(GtkTreeStore*, GtkTreeIter*, std::string)'

Please apply clang format
The icons you use (for adding a folder, closing all folders) are not in the Adwaita icon theme. So you should add icons to the ui folder.
Rnote also has a workspace browser (an already quite refined one), maybe also inspired from Notepad++. You might want to take some inspiration from there as well.

Febbe · 2023-10-02T21:21:06Z

src/core/control/Control.h

@@ -392,9 +398,11 @@ class Control:

    Document* doc = nullptr;

+    Workspace* workspace = nullptr;


Do not use raw pointers for owned resources, instead use std::unique_ptr.

Suggested change

Workspace* workspace = nullptr;

std::unique_ptr<Workspace> workspace;

Febbe · 2023-10-02T21:21:39Z

src/core/control/Control.h

    Sidebar* sidebar = nullptr;
    SearchBar* searchBar = nullptr;

+    WorkspaceHandler* workspaceHandler;


The same applies here, additionally, all (new) members should be initialized.

Suggested change

WorkspaceHandler* workspaceHandler;

std::unique_ptr<WorkspaceHandler> workspaceHandler;

Febbe · 2023-10-02T21:34:21Z

src/core/control/workspace/WorkspaceHandler.cpp

+WorkspaceHandler::~WorkspaceHandler() {
+	this->removeAllListeners();
+}


When there is no WorkspaceHandler, we also don't have any handler left.
Also, destructors + special member functions should be defaulted whenever possible.

Suggested change

WorkspaceHandler::~WorkspaceHandler() {

this->removeAllListeners();

}

WorkspaceHandler::~WorkspaceHandler() = default;

Febbe · 2023-10-02T21:37:01Z

src/core/control/stockdlg/XojOpenDlg.cpp

+auto XojOpenDlg::showOpenFolderDialog() -> fs::path {
+
+    gtk_file_chooser_set_action(GTK_FILE_CHOOSER(this->dialog), GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER);
+
+    GtkWidget* image = gtk_image_new();
+    gtk_file_chooser_set_preview_widget(GTK_FILE_CHOOSER(this->dialog), image);
+    g_signal_connect(dialog, "update-preview", G_CALLBACK(updatePreviewCallback), nullptr);
+
+    auto lastOpenPath = this->settings->getLastOpenPath();
+    if (!lastOpenPath.empty()) {
+        gtk_file_chooser_set_current_folder(GTK_FILE_CHOOSER(this->dialog), Util::toGFilename(lastOpenPath).c_str());
+    }
+
+    auto lastSavePath = this->settings->getLastSavePath();
+    if (!lastSavePath.empty()) {
+        gtk_file_chooser_add_shortcut_folder(GTK_FILE_CHOOSER(this->dialog), lastSavePath.u8string().c_str(), nullptr);
+    }
+
+    fs::path folder = runDialog();
+
+    if (!folder.empty()) {
+        g_message("lastOpenPath set");
+        this->settings->setLastOpenPath(folder);
+    }
+
+
+    return folder;
+}
+


Nothing, you can probably change for now, but we must refactor this into some sort of async operation. Either hard-coded to std::future or using something like asio's completion handlers (e.g. use_awaitable).

Febbe · 2023-10-02T21:50:37Z

src/core/control/workspace/WorkspaceHandler.h

+    void load();
+    void save();
+
+    std::set<fs::path> getFoldersPaths();


We don't want to copy containers when possible. They should be returned as const &. The implementation can still decide if it wants to copy it.

Also, a std::vector might be the better choice, since it is not a tree and a contiguous container (std::flat_set/boost::flat_set are desirable, but we don't have them).

Suggested change

std::set<fs::path> getFoldersPaths();

auto getFoldersPaths() -> std::vector<fs::path> const&;

Febbe · 2023-10-02T22:26:31Z

src/core/gui/Workspace.cpp

+        gtk_tree_model_get(GTK_TREE_MODEL(model), &iter, COL_FULLPATH, &str, -1);
+
+        fs::path fsPath;
+        if (str != nullptr) // root folder


str == nullptr should be an error. Or you should exit early.
The root / current folder should be an empty string, which is not nullptr.

Febbe · 2023-10-02T22:36:57Z

src/core/gui/Workspace.cpp

+
+    GtkTreeIter iter;
+    gtk_tree_store_append(store, &iter, nullptr);
+    gtk_tree_store_set(store, &iter, COL_NAME, fs::path(folderPath).filename().c_str(), -1);


We want the content of the tree to be encoded in utf8, since the whole gtk gui expects utf8 strings.

Suggested change

gtk_tree_store_set(store, &iter, COL_NAME, fs::path(folderPath).filename().c_str(), -1);

gtk_tree_store_set(store, &iter, COL_NAME, fs::path(folderPath).filename().u8string().c_str(), -1);

Febbe · 2023-10-02T22:44:47Z

src/core/gui/Workspace.cpp

+void Workspace::createViewAndStore() {
+
+    treeView = gtk_tree_view_new();
+
+    g_signal_connect(treeView, "row-activated", G_CALLBACK(treeViewRowClicked), this);
+
+    GtkCellRenderer* renderer;
+
+    renderer = gtk_cell_renderer_text_new();
+    gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(treeView), -1, "Name", renderer, "text", 0, nullptr);
+
+    gtk_tree_view_set_headers_visible(GTK_TREE_VIEW(treeView), false);
+
+    store = gtk_tree_store_new(NUM_COLS, G_TYPE_STRING, G_TYPE_STRING);
+    GtkTreeModel* model = GTK_TREE_MODEL(store);
+    gtk_tree_view_set_model(GTK_TREE_VIEW(treeView), model);
+    g_object_unref(model);
+}


It looks like this function is only used in the constructor of workspace. Therefore, it should be linked static (static or anonymous namespace) in the source file, also return the tree view widget.

Febbe · 2023-10-02T22:46:26Z

src/core/gui/Workspace.cpp

+
+    this->workspaceSidebar = gui->get("workspaceSidebar");
+
+    this->createViewAndStore();


Suggested change

this->createViewAndStore();

auto* treeView = this->createViewAndStore();

Febbe · 2023-10-02T22:47:40Z

src/core/gui/Workspace.cpp

+        GtkTreeIter iter;
+        if (entry.is_directory() || exts.find(entryPath.extension()) != exts.end()) {
+            gtk_tree_store_append(store, &iter, parent);
+            gtk_tree_store_set(store, &iter, COL_NAME, entryFilename.generic_string().c_str(), COL_FULLPATH, entryPath.c_str(), -1);


Suggested change

gtk_tree_store_set(store, &iter, COL_NAME, entryFilename.generic_string().c_str(), COL_FULLPATH, entryPath.c_str(), -1);

gtk_tree_store_set(store, &iter, COL_NAME, entryFilename.generic_u8string().c_str(), COL_FULLPATH, entryPath.u8string().c_str(), -1);

rolandlo · 2023-10-28T14:01:33Z

One question about the design: Instead of creating a separate sidebar for the workspace, wouldn't it make more sense to present it in a way that you can switch between preview and workspace (and something like "library" in future), but not see both at once? Here is a mockup from the Gtk4/Libadwaita redesign

jonjampen · 2024-03-04T08:15:02Z

This is exactly the feature I was looking for! Any updates on this?

MatteoGalletta added 2 commits October 1, 2023 11:47

Workspace sidebar

c577702

Workspace sidebar

d6b7d5d

MatteoGalletta marked this pull request as draft October 2, 2023 10:39

Febbe requested changes Oct 2, 2023

View reviewed changes

netlimpopo mentioned this pull request Oct 24, 2023

Start page with recently used documents #4945

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workspace sidebar #5189

Workspace sidebar #5189

MatteoGalletta commented Oct 1, 2023

rolandlo commented Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

Febbe Oct 2, 2023

rolandlo commented Oct 28, 2023

jonjampen commented Mar 4, 2024

		@@ -392,9 +398,11 @@ class Control:

		Document* doc = nullptr;

		Workspace* workspace = nullptr;

	Workspace* workspace = nullptr;
	std::unique_ptr<Workspace> workspace;

	WorkspaceHandler* workspaceHandler;
	std::unique_ptr<WorkspaceHandler> workspaceHandler;

	std::set<fs::path> getFoldersPaths();
	auto getFoldersPaths() -> std::vector<fs::path> const&;

	gtk_tree_store_set(store, &iter, COL_NAME, fs::path(folderPath).filename().c_str(), -1);
	gtk_tree_store_set(store, &iter, COL_NAME, fs::path(folderPath).filename().u8string().c_str(), -1);


		this->workspaceSidebar = gui->get("workspaceSidebar");

		this->createViewAndStore();

	this->createViewAndStore();
	auto* treeView = this->createViewAndStore();

Workspace sidebar #5189

Are you sure you want to change the base?

Workspace sidebar #5189

Conversation

MatteoGalletta commented Oct 1, 2023

rolandlo commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolandlo commented Oct 28, 2023

jonjampen commented Mar 4, 2024