Merge lp://qastaging/~matzipan/scratch/folder-manager-improvement into lp://qastaging/~elementary-apps/scratch/scratch

Couple code style comments. If you're checking the same variable more than twice, make sure to use a switch/case statement :) https://elementary.io/docs/code/reference#indentation

On compiling there is a warning:
"warning: method `Scratch.Plugins.FolderManager.File.reset_cache' never used".

See also inline comments.

Function review to follow.

There are some issues in function - All but the first two may be considered outside the scope of this branch:

This branch:
1) rename a file causes another document to load because cursor does not follow (and, if open, file in document view not updated with new name)
2) Second opened folder saved in settings but not restored (removed on restarting)

Others?:
3) "New Folder" different from Files "untitled folder"
4) "New File" different from Files "new file".
5) Expanded status not saved in settings.

For 1 I have a nother branch [1] which fixes the issue, however, there is a limitation in Granite.SourceList which prevents elements from being sorted when they are being renamed in the list. I don't want to code a workaround in this branch since it will be fixed anyway.

Investigating 2.

5 would be outside of the scope of this branch. I believe trunk didn't do that either.

[1] https://code.launchpad.net/~matzipan/scratch/use-rename-event/+merge/312213

One response to your comment inline. I basically just copied the code over from the original implementation. I will address it.

Issues were addressed.

Some further comments on the code (not necessarily blocking):

1) The class "Scratch.Plugins.FolderManager.Item" is described as abstract but not defined as such and it would be better if a public class had its own file.

2) The reference to "view" is not really required in "Item", only in "FolderItem" (and is it necessary to have a back reference to the Item's parent?).

3) The function "on_child_removed" is not currently used

Also discovered an issue with deleting an opened folder (it doesn't remove the sidebar entry, it only trashes the file).

And there seems to be a problem with opening empty folders.

Addressed 1 and 3.

2 isn't true. FileItem uses view for view.start_editing_item (this).

I know it's bad practice, but I don't want to make it this branches' scope to refactor everything. I just wanted to get the basic usecases working.

I also fixed an issue where it wouldn't save to settings when the folder is closed.

Current problems:
* when a new folder or file is created, it only shows the editable entry for a short time.
* there seems to be a problem with opening empty folders.

Just realized that SourceList headers are not visible when they have no children. I don't think there's anything I can change about that.

I will add a check against this.

I've spent 3 hours on the issue "when a new folder or file is created, it only shows the editable entry for a short time.". I could not tell why this is happening. I added a Idle.add() workaround and it worked like this.

Dan thinks this merge request is too large. Unfortunately, I don't think I can break this down into smaller chunks - I don't wanna be committing something I know wouldn't work. And I cannot get it smaller.

I agree that it is not easy to split this kind of branch, and you are saving 500 lines of code so it is worth the extra effort of checking for regressions.

The comment about back references is not critical and can be disregarded at least in the branch.

One of the revisions added the "new" keyword. Is there a reason you did this rather than mark the corresponding function in the abstract class "Item" as virtual?

There are still some issues with folder items that have just been created (using the folder manager plugin):

A
 1) Create a folder item in the root folder
 2) Immediately create another folder within that new folder

 Two subfolder items appear even though only one subfolder is created on disk. Deleting one of them causes both to disappear.

B
 1) Create a folder item in the root folder
 2) Immediately create a new file within that new folder - two file entries appear
 3) Delete one file entry - both disappear
 4) Create a new folder - one folder entry and one file entry appear
 5) Delete the file entry - only the file entry disappears.

The double creation effect is due to the FileMonitor detecting the creation and adding another item. You need to block the monitor while creating an item internally.

Ignore last comment - it seems that is not the problem

> One of the revisions added the "new" keyword. Is there a reason you did this rather than mark the corresponding function in the abstract class "Item" as virtual?

Initially I used "override", however, it seems "override" in Vala is not the same as in Java, so I removed it, and then the compiler noticed my declaration is shadowing a function in the parent class, and suggested I add "new" if this is intentional.

It's ready for review again.

I jotted down 2 future improvements here: https://bugs.launchpad.net/scratch/+bug/1646924

The last revisions fixes the duplicate creation problem. There is a small bug - the "newly_created_path" variable should be nulled after it has served its purpose. If not, if you create a new file/folder with the folder-manager and then delete it and recreate it with Files, both Files and the folder-manager go into edit mode. Better to check whether it is null before using it in an equality.

I have confirmed the future improvement bug.

Thanks for catching that one. That's how I was intending to do it, but between my brain and my hands something got lost.

I believe the null check is not necessary. It seems to be running alright on my end.

I agree Vala does not mind nulls in equalities - it just felt wrong - and it may be a little more efficient to avoid invoking source.get_path () but it is not critical.

I'll give it another test tomorrow but it is looking good now.

If the "execute" permission is removed from a folder then the "add file" and "add folder" options are still active. If these are chosen then scratch hangs while maxing out a core. (Under these circumstances, Files gives a "permission denied" error dialog. However, if the folder is set to "read only" but has "execute" permission, attempting to add a file fails silently (with a error message in the terminal). Under these circumstances Files does not offer to create a file in the context menu.

I also notices that "Move to trash" is offered for all folders including the user home folder. Hopefully this would fail but I did not feel like trying it (!).

I discovered one more issue, if I compile a C++ file with g++ bla.c, a.out shows up (unexpected). If I close Scratch and open it back up, the file doesn't show up anymore (expected).

Also if I click the a.out entry, it freezes my editor.

It's not clear to me why this happens.

Ok, so the above commit checks if the folder is executable before checking for file existence to avoid infinite looping.

However, there is much more involved work necessary to allow all files to track their attributes, like File does. For this, I have created the bug report [1].

For the "move to trash issue" I added a note in [2].

[1] https://bugs.launchpad.net/scratch/+bug/1650902
[2] https://bugs.launchpad.net/scratch/+bug/1646924

Ok, I notice the a.out issue I reported above is a regression in trunk too, so I'll file a bug for this, since it's outside the scope of this branch.

Found an issue: when you create a folder in Files, it gets created in Scratch just fine. If you create a file in that folder in Files, the folder should be marked as expandable in Scratch, but that does not happen.

Fixed.

I get a crash under the following conditions:

1. Open Scratch with the folder manager showing a folder.

2. Using Files, copy another folder in the folder that is open in Scratch.

3. Scratch crashes.

Note: The crash mention above does not occur if the folder is empty or contains an empty folder.

I have fixed the raised issue. It was introduced in my last commit, I didn't check everything - sorry for that.

I also added some improvements for unloaded folders and folder children validity.

Yeah, I also tested it. The issue described in https://code.launchpad.net/~matzipan/scratch/folder-manager-improvement/+merge/312195/comments/814590 is fixed with r 1797.

Another question: is it intended that one has to select both plugins (file manager and folder manager) in the plugins menu to activate the side bar?

The lastest revisions fix the crash previously noted, so I think this now good to go :)

The "File Manager" extension will be no longer needed once this is merged but will still be installed on some users systems and therefore will still appear in the extension list. I am not sure whether an update can uninstall this extension or whether it will need to be "blacklisted" so that it does not show. That can be done in another branch.

@Ralph: this is a caused because the old plugin that was merged into this one was never deleted upon install. I'm not sure how to deal with this. Maybe Dan or Cody can suggest.

@Ralph: You should be able to safely delete (or move elsewhere if you want to be able to reverse it) the folder "filemanager" in /usr/lib/x86_64-linux-gnu/scratch/plugins/ once it is no longer needed. You will need to be root.

This will remove the plugin from the list of available plugins in Scratch.