Rework of VFS (Now: FileSystem)
Closed, ResolvedPublic

Description

I'm currently working on cleaning up the VFS code.
It's a mess that evolved over the last 10 years; some parts are using KIO, some other Qt functions and a big factor is the support for a total custom virt:// filesystem that makes everything much more complicated.

Here are some ideas and notes mainly for myself (WIP):

  • there are currently three implementations of the vfs class:
    • normal_vfs for the local "file:/" filesystem
    • ftp_vfs for ftp:// and other KIO protocls (smb:/, fish:/)
    • virt_vfs for the virtual filesystem: list of files which are only references to real files. Preserved even when restarting Krusader. Used at least for bookmarks, locate, and searchdialog
  • big question is what to do with virt_vfs. Dumping it is probably not possible without reworking all parts that are using it. But I hope I can simplify huge parts of it.

Maybe to remove (?):

  • merging normal_vfs with ftp_vfs
  • preservingcopyjob has no real purpose
  • Queue: a list dialog for KIO jobs? Is somebody really using this? Nowadays every DE has it's own one
  • virtualcopyjob is not even used in virt_vfs!? Only with queue

These are all random thought for the record. Feel free to comment if you have own ideas, thoughts or you think I'm on the wrong track!

DONE:

  • removed: Queue/*, kiojobwrapper, virtualcopyjob, preservingcopyjob
  • merged ftp_vfs with normal_vfs to default_vfs
  • rework of vfs, default_vfs and virt_vfs
  • cleaned up panelfunc
  • krvfshandler: connect to vfs instances
  • added a job queue replacement: JobMan
  • New own SizeCalculation class

TODOs:

  • vfilecontainer: merge with vfs

Eternal TODO:

  • testing, testing and testing

Details

Differential Revisions
D3025: Rework and cleanup of VFS
abika created this task.Aug 9 2016, 2:21 PM

Toni, thanks a lot for your work and effort! I'm not quite in detail so I basically agree with everything except the Queue. I actually use it from time to time when I want to iteratively select and copy many files to e.g. remote location with slow transport speed.

But You are saying that KDE has some alternative?

abika claimed this task.Aug 10 2016, 9:50 PM

It's me, Alex.) But no big deal.

In KDE you have the notification area for all long IO operations where you can see the progress and you can pause/resume/stop a job (unfortunately they are not queued).
And I already deleted Krusader's queue dialog in my working branch. Using it is quite complicated and it wasn't possible to add all jobs (e.g. started with drag'n'drop) to it.

But you are right! Having an overview over all copy/move operations with progress dialog is really useful.

My plan is to re-add some sort of drop-down menu where ALL jobs started in Krusader are listed automatically. Together with pause/resume/stop and maybe even undo options.

Alex, I'm very sorry about the "Toni". I quite often switch "abika" and "asensi" in my mind and then correct myself but this time I didn't.

About the Queue. Ok now I see - the overview in KDE notfications, that I actually know. I meant mainly the queuing functionality. Total Commander has the same thing - e.g. when copy dialog shows up, you have the option to hit F2 and the copy task is queued.

But since there is no drop support we can either offer queuing without drop support or wait for KIO to support this. I also think the current Queue was not ideal and I'll be happy to reimplement it if we agree on that :).

asensi added a subscriber: asensi.EditedAug 13 2016, 10:55 AM

I agree, too, and maybe with those VFS improvements... important bugs like

A problem renaming a file that is inside an archive
https://bugs.kde.org/show_bug.cgi?id=350028

would be solved.

abika added a comment.Aug 13 2016, 6:41 PM

Unfortunately, not this one.
Just checked it, renaming is done (before and after my patch) with

KIO::Job *job = KIO::moveAs(oldUrl, newUrl, KIO::HideProgressInfo);

The URL arguments look good so it very likely a problem with krarc. And I don't intend to touch anything related to krarc or archive handling.
This does not happen in Dolphin because it detects that files in an archive are not movable. This is not really true but disables renaming in the view.

[Offtopic:
It is somehow possible to get email notifications for any new bug reports or bug changes for Krusader on bugs.kde.org?
]

abika updated the task description. (Show Details)Aug 13 2016, 6:57 PM
abika updated the task description. (Show Details)Aug 23 2016, 3:00 PM
abika added a comment.Aug 23 2016, 3:05 PM

I would also like to remove KrCalcSpaceDialog, a popup dialog showing files, folders and space usage of a selected directory (ctrl+shift+z).
The same information is included in the "Properties" context dialog and here it also works for non-local filesystems. I don't the the point for having two dialogs with the same information.

Any objections?

No objection. I think it's a good idea. And thanks for all the hard work, Alex!

In T3419#48717, @asensi wrote:

I agree, too, and maybe with those VFS improvements... important bugs like

A problem renaming a file that is inside an archive
https://bugs.kde.org/show_bug.cgi?id=350028

would be solved.

I'm (almost) working on it in T3479. A new method krarc::rename has to be written (I will dive into Ark code).

On the main topic, VFS need some work. But I wouldn't remove Queue: it's handy when you have to move many file over a network (jobs started sequentially). When I mount a NFS over the VPN, starting all transfers togheter could kill the connection.

OT @abika: for Bugzilla, in preferences->email I put "mg@fork.pl " (which is the default assignee for Krusader - don't ask why) to the User Watching, it works.

OT @gengisdave :

for Bugzilla, in preferences->email I put "mg@fork.pl " (which is the default assignee
for Krusader - don't ask why) to the User Watching, it works.

Thanks, Davide!

Alex wrote:

I would also like to remove KrCalcSpaceDialog, a popup dialog showing files,
folders and space usage of a selected directory (ctrl+shift+z).
The same information is included in the "Properties" context dialog and here it also
works for non-local filesystems. I don't the the point for having two dialogs with the
same information.

I agree with Alex and Martin :-)

Note: I press ctrl+shift+z and nothing happens, I've looked for it in the shortcuts list, but doesn't appear there. To use the feature, I press the secondary button of the mouse and select «Calculate Occupied Space».

Thanks, Alex!

I agree too. The KrCalcSpaceDialog pops up after a while when calculating directory space. It is a way to know that Krusader isn't freezed. When done, the popup closes and the <DIR> string is changed with the dir space. Anything to say "hey, I'm working, but I'm not freezed" is good to; like when krarc reads a file.

asensi added a comment.EditedAug 24 2016, 8:28 AM

After trying that feature in Total Commander and Double Commander:
I'm thinking about people that use the space key to select several folders and see how much each folder occupies, at a single glance. Then those people are able to sort the folders by size using the "Size" column header, etc. If Krusader already has this functionality, and also Double Commander and Total Commander have it too (some people have to use both Linux and Windows), if it doesn't cause a big problem, probably we should leave this feature working.

As Davide wrote about the KrCalcSpaceDialog:

anything to say "hey, I'm working, but I'm not freezed" is good too.

abika added a comment.Aug 24 2016, 7:33 PM

I didn't see that this feature also sets the directory sizes in the current view, pretty nice! Unfortunately this happens also in KrCalcSpaceDialog so I will leave it as it is.

Thanks to all of you!

abika added a comment.Oct 11 2016, 4:26 PM

late but not forgotten.

Summary:

  • > 89 files changed, 1707 insertions(+), 4883 deletions(-)
  • a lot more documentation and more consistent interface for using the vfs classes
  • the job queue was completely removed and I intend to re-implement it later on
  • some TODOs are left; the comment lines are all inserted by a separate commit. I found many (previous) bugs during the rework, some of them are fixed but some are also left as further work
  • some reported bugs on Bugzilla are fixed by this, at least https://bugs.kde.org/show_bug.cgi?id=368450 . I have to check for more...
abika added a comment.Oct 11 2016, 4:34 PM

And pushed the entire branch to make testing easier.

I (and hopefully others) will further use it for testing. This will definitely not go into master soon.

Thanks a lot for this extensive work, Alex! I'll be using this branch as my default now.

abika added a comment.Oct 12 2016, 1:42 PM

Thanks Martin!

I already noticed that refreshing (which also happens on all file operations) is really slower. At first I thought its not that bad but its really noticeable. Have to fix that...

You're right, now I'm now noticing that, too. It looks like it is slow even with e.g. 2 folders, so it looks like some kind of "fixed" delay when refreshing.

I can see another minor problem: after renaming file/folder and refresh is performed, focus from renamed item is lost (moved to "..").

abika added a comment.Oct 17 2016, 3:08 PM

You're right, now I'm now noticing that, too. It looks like it is slow even with e.g. 2 folders, so it looks like some kind of "fixed" delay when refreshing.

You were absolutely right about the fixed delay. It way caused by the filesystem watcher which sends signals for changed files with a delay of 1/2 seconds. It should be fixed now.

I can see another minor problem: after renaming file/folder and refresh is performed, focus from renamed item is lost (moved to "..").

Yes, that needs to be fixed next. Thanks for testing and reporting Martin, it is a great help!

I noticed you moved a commit to this branch. Please don't do this! Rebased commits are treated as different commits to the originals one (the SHA hash is different) and makes it impossible to merge this branch back to master once it is final without double commits.
You can simply create a local branch and merge from master and vfs_rework.
I had to remove and recreate the branch (git push -f is not allowed). So be aware of conflicts if you want to pull.

And Phabricator is always closing the Differential for this Task. I'm tired of dealing with it and will do all further changes directly in git without arc and Phabricator.

It should be fixed now.

Now I can see no delays, thanks Alex!

I noticed you moved a commit to this branch. Please don't do this!

I'm very sorry! I'm still a beginner in these things. Thanks a lot for the explanation - now I understand much better how merging of branches work. Next time I decide to do something new, I'll better ask first :-).

I'm tired of dealing with it and will do all further changes directly in git without arc and Phabricator.

Sure, no problem.

abika added a comment.Oct 18 2016, 4:41 PM

New commits. Loosing the current selection after rename is fixed now, too. If you find anything else don't hesitate to report it. Thanks!

I'm very sorry! I'm still a beginner in these things. Thanks a lot for the explanation - now I understand much better how merging of branches work. Next time I decide to do something new, I'll better ask first :-).

No big deal. If its not about the master branch it doesn't matter much.
And yes, Git is hard to understand and even harder to master. I have still problems myself. An example for a good branching model is here: http://nvie.com/posts/a-successful-git-branching-model/

An example for a good branching model is here: http://nvie.com/posts/a-successful-git-branching-model/

Thanks for the useful link!

If you find anything else don't hesitate to report it.

Here are today's findings (not regressions of the latest commits):

  • when focused item is externally renamed or removed, focus is lost (moved to "..")
    • master version (of Krusader) in these cases moves cursor to the next item
    • nice to have: keep focus on renamed item (dolphin manages to do it somehow - maybe by inode?)
  • when pasting in folder or deleting from it, free space info (in the bottom right of Krusader) is not refreshed and Ctrl+R is not refreshing it either

Thanks again!

I forgot to say that loosing current selection after rename is now truly fixed. Thanks!

abika added a comment.Oct 24 2016, 3:13 PM
  • when focused item is externally renamed or removed, focus is lost (moved to "..")
    • master version (of Krusader) in these cases moves cursor to the next item
    • nice to have: keep focus on renamed item (dolphin manages to do it somehow - maybe by inode?)

I have to check how easy it is to implement this. But I'll give it low priority if you don't mind.

  • when pasting in folder or deleting from it, free space info (in the bottom right of Krusader) is not refreshed and Ctrl+R is not refreshing it either

My fault, should be fixed now.

Confirmed, free space info is now correctly refreshed, thanks Alex!

About the refreshing - I have some additional info. I try to summarize it:

  • any refresh of item list make a scroll jump up when it was previously scrolled down
  • if I rename an item in Krusader, this item is now "remembered"
    • then, regardless of focused item, when external change of anything is triggered, the remembered item is focused again (unless this remembered item was externally renamed/removed -> this way focus is moved to "..")

I have to check how easy it is to implement this. But I'll give it low priority if you don't mind.

I definitely don't mind low priority, You have clearly your hands full:). I try to make some more time this week, study the code and fix it myself.

OK, here is my first attempt for fixing the scroll jump and cursor move.

Basically I've done this:

  • revert of fileDeleted/delItem functions removal
  • introducing of _pendingVfiles (they are meant to be used only when refreshing)
  • created vfs::storeAndEmitUpdates(...) method for determining actual changes between _vfiles and _pendingVfiles and emitting selective change signals (add/update/delete)
    • this function is kind of stolen from vfs::vfs_refresh() of master branch
  • off-topic change: vfs::clear(vfileDict vfiles) -> vfs::clear(vfileDict &vfiles) to actually clear the hashmap keys

Please don't hesitate to criticize and/or even throw away this whole solution of mine. I'm not sure if I've picked the right path to achieve meant results.

I've corrected a crash bug in my code and created a proper revision:
https://phabricator.kde.org/D3208
So please forgot the attachment in my previous post.

abika added a comment.Oct 31 2016, 4:37 PM

Well, thanks for the patch. But I really think these are problems with the view, not the VFS. I try to remove as much unneeded code as possible. The VFS is not pretty clean not but the Panel (especially PanelFunc and KrView and subclasses) is simply not updated everywhere to reflect the changes.

  • if I rename an item in Krusader, this item is now "remembered"
    • then, regardless of focused item, when external change of anything is triggered, the remembered item is focused again (unless this remembered item was externally renamed/removed -> this way focus is moved to "..")

This was a bug in the panel. Fixed now.

  • any refresh of item list make a scroll jump up when it was previously scrolled down

Scrolling is done to the current selected item (please correct me if you have different behaviour). You could call it a feature: if the file list really changed (files added or removed) you want to keep the current item visible and the scrolling must be adjusted.

But this if of course arguable (or a matter of taste). In case the current item is the first (default) one and you scroll way done to the end, you don't want to end up at the top every time some file changed.
I (or you if you like) can change that. But this simply belongs to the view (=panel section), not to the VFS.

  • off-topic change: vfs::clear(vfileDict vfiles) -> vfs::clear(vfileDict &vfiles) to actually clear the hashmap keys

Damn, Thanks! Im doing too much Java to notice such stupidity:)

But I really think these are problems with the view, not the VFS.

You're probably right. Here are my thoughts that led me to my "solution". I think that the selective events for file creation and deletion should actually be provided by KDirWatch. Since this is not quite possible and we have to rely only on a dirty signal, I suspected the next layer (VFS) should handle that and emit granular changes to view. So view can only redraw what really changed.

But as I said, You're right, let's keep VFS as clean as possible and if necessary let's handle it in view.

This was a bug in the panel. Fixed now.

Perfect, thanks! This is now working neatly. But You also fixed the the scroll jumps that I meant before!

Scrolling is done to the current selected item. You could call it a feature...

I actually don't mind these, it is probably a feature. Before, I meant scroll jumps when current item was still visible before refresh, but scroll was adjusted anyway after refresh. That is now fixed, thanks!

The only thing left to fix (from my point of view) is the external rename of focused item. When this happens now, no item is focused anymore and up/down arrow buttons do not bring any item focus back. I'll try to fix that during this week.

Damn, Thanks! Im doing too much Java to notice such stupidity:)

IMO there is no stupidity in that. I'm mostly java programmer as well so I've learnt this one the hard way:).

Here is a new patch for the view :). Should address the external rename/remove. Again, don't hesitate to throw it away.
https://phabricator.kde.org/D3237

abika removed abika as the assignee of this task.Nov 3 2016, 8:24 PM

I added a first version of a new queue manager. "JobMan", used for all jobs, this will give the user general info and control over file operations if not provided/activated in desktop environment (see https://bugs.kde.org/show_bug.cgi?id=356697)

  • UI part are toolbar actions. Visible via settings->toolbars->show job toolbar
  • progress not visible for trash and drag/drop jobs (TODO)
  • queue option is added again but job behaviour is buggy (TODO)

Criticism is welcome.

Screenshot:

abika claimed this task.Nov 3 2016, 8:26 PM

ehh, whatever.

Very good, Alex thanks!

I like the idea of toolbar integration! Right know I can see these minor issues with it:

  • when right-clicking on toolbar -> Toolbar Settings... I am unable to actually configure the new Job Toolbar; Job Toolbar is therefore always empty
  • Job Progress Bar is not shown correctly (I can see only the title "Job Progress Bar") when added to Actions Toolbar
  • right-clicking next to menubar shows popup with enabled toolbars, one of them has missing title (probably Job Toolbar)
  • hiding Job Toolbar does not survive Krusader restart
  • when starting Krusader fresh (with non-existing krusaderrc) both Actions and Main Toolbars have Job toolbar items added
  • clicking on Job List icon does nothing, only the arrow next to it opens the overview popup
  • when no file transfer is happening, progressbar is greyed out (thats OK) with the last shown percentage and sometimes this percentage is not 100% although the transfer finished successfully

Sorry about the long list. Anyways I'm very happy we are going to have JobMan in Krusader now.

abika added a comment.Nov 9 2016, 8:21 PM
  • when right-clicking on toolbar -> Toolbar Settings... I am unable to actually configure the new Job Toolbar; Job Toolbar is therefore always empty.

Right-click->"Configure toolbars"? "Toolbar Settings" is only the title and never does anything.

And see below.

  • Job Progress Bar is not shown correctly (I can see only the title "Job Progress Bar") when added to Actions Toolbar

I can only confirm this is if the "Job Progress bar" is not already in another toolbar. Looks like the progress bar action can only be shown once (it works fine for all other actions/buttons).

  • right-clicking next to menubar shows popup with enabled toolbars, one of them has missing title (probably Job Toolbar)

See below.

  • hiding Job Toolbar does not survive Krusader restart

I thought saving/restore toolbar settings is done automatically by KXMLGuiWindow. It's not (setupGUI() is not used) and fixed now.

  • when starting Krusader fresh (with non-existing krusaderrc) both Actions and Main Toolbars have Job toolbar items added

See below.

  • clicking on Job List icon does nothing, only the arrow next to it opens the overview popup

I thought more about his and made a rework: One "control" button for play/pause/clear all jobs and with drop down menu. Each job entry in the menu with an own play/pause and cancel button.
And progress bar is accumulated progress of all jobs.

  • when no file transfer is happening, progressbar is greyed out (thats OK) with the last shown percentage and sometimes this percentage is not 100% although the transfer finished successfully

Ok, sometimes the last "100 percent" signal is not emitted and the progress bar must be set manually to 100. Fixed.

Sorry about the long list.

Don't be! You're helping me a lot with your feedback, that's exactly what I'm hoping for.


About the toolbar:
Problems with it (missing titles, etc.) are maybe related to the *ui.rc file. See https://docs.kde.org/trunk5/en/extragear-utils/krusader/faq.html#idp74195328

The correct (new) version number is version="24".

Hope, that will solve it.

Hi Alex! Thanks a lot for your fixes and advices!

Right-click->"Configure toolbars"? "Toolbar Settings" is only the title and never does anything.

Sorry about that, you're right of course. Anyway, all my toolbar problems disappeared after removing ~/.local/share/kxmlgui5/krusader/ directory. You are a great detective, we should switch avatars:).

After your fixes the progressbar, buttons, overview looks definitely better, more intuitive. I like it and have no additional suggestions. Thanks! Here are some very small issues:

  1. Sometimes I can still see numbers below 100% next to successfully finished jobs (see screenshot below).
  2. Deleted item has "-" instead of its location displayed in overview (see screenshot below).

Over the weeks I've noticed a few more small issues about the reworked vfs but I had to test more to be more deterministic:
I.) When remote location is requested in a tab and this tab is closed before request finishes, it crashes Krusader.
II.) Now here is probably a problem/feature of KIO exposed by new vfs code - steps to reproduce:

  1. I connect to a SFTP location with root user. This way I'm always asked for password (even though it is pre-filled from kwallet). Probably a security feature of KIO, because I don't have this issue with another SFTP servers using different usernames.
  2. With new vfs code SFTP connection is refreshed in some cases and during these moments I'm asked for the pre-filled password again:
    • [1. issue] Remote file search (Ctrl+S) with containing text -> after the first found file sftp.so process is replaced by another one and at this moment I have to confirm the pre-filled password again. This happens only after the first found file; but every time new search is commenced from search dialog.
    • [2. issue] Deletion of remote folder containing some folders and files -> it asks for pre-filled password and after confirming it, ends with error in this panel anyway. This happens often, not every time; a Krusader restart + direct opening of given location + removal of the folder is usually successful after that.
    • I will investigate the problem further and if I come up with anything, I will let You know.

One more issue, not a vfs_rework regression:
When a location is loading in e.g. right pane and I switch to the left one, the right one is forcibly focused when the loading is finished. Do we want that? If no, I try to fix it.

abika added a comment.Nov 14 2016, 7:27 PM
  1. Sometimes I can still see numbers below 100% next to successfully finished jobs (see screenshot below).
  2. Deleted item has "-" instead of its location displayed in overview (see screenshot below).

Both fixed now.

I.) When remote location is requested in a tab and this tab is closed before request finishes, it crashes Krusader.

Fixed. vfs was doing fine but panel did not consider it could be deleted while refreshing:)

II.) Now here is probably a problem/feature of KIO exposed by new vfs code - steps to reproduce:
...

I'll have to check this when i have more time. Passwords with (S)FTP are a general problem (https://bugs.kde.org/show_bug.cgi?id=335668 is still not solved). And why the refreshing is done with the new VFS code.

One more issue, not a vfs_rework regression:
When a location is loading in e.g. right pane and I switch to the left one, the right one is forcibly focused when the loading is finished. Do we want that? If no, I try to fix it.

I guess not. It should be fixable with one or two lines (?)


I also added a "job mode" button. But the current behavior is totally defective because jobs cannot be started in pause mode. Hope, the frameworks devs are helping out...

Perfect! Thanks for all the fixes!

About the KIO password problem: it is not the case with visible passwords. I got lucky and found the cause of my problem by comparing vfs_rework and master code - see D3360. Magic. At least for me. KIO apparently remembers the passwords when the dialog is connected to parent window and then never asks for it again during Krusader run, which is desirable.

It should be fixable with one or two lines (?)

Probably. I'll do that soon.

I also added a "job mode" button. But the current behavior is totally defective because jobs cannot be started in pause mode. Hope, the frameworks devs are helping out...

Interesting! Although currently I cannot think of a use-case when somebody wants to start every job paused. I'd also make the "Unmanaged" mode a default. It is definitely good to have "Queue" mode as an option, still with the possibility to enforce it on demand by F<NUM> -> F2 combo.

But maybe I miss the point of job modes. How are they supposed to work exactly? Should they apply for all file operations started in Krusader?

abika added a comment.Nov 15 2016, 7:18 PM

About the KIO password problem: it is not the case with visible passwords. I got lucky and found the cause of my problem by comparing vfs_rework and master code - see D3360. Magic. At least for me.

Magic to me, too. That's why I removed it.) KIO is often unclear about what happens internally. But thanks for working this out.

I'd also make the "Unmanaged" mode a default.

I politely disagree. It is true that "unmanaged" corresponds to the the current behaviour- just start all jobs right away. But during testing i often had the situation that my system slows down a lot when more than two copy operations are working in parallel. And I believe most users will expect the queue behaviour.

Anyway, adding a config setting for this is also an option. And I hope we get more user opinions and feedback if the branch is merged with the master. I'm still open to everything.

But maybe I miss the point of job modes. How are they supposed to work exactly? Should they apply for all file operations started in Krusader?

Apparently some users were using the queue manager for predefining copy and move jobs and delaying those batch operations to be executed later - in combination with (ftp) servers this makes sense. But I'm never doing this myself and hope the new job manager meets the requirements.


New feature: an "undo button" for undoing the last copy/move/link/rename/trash job.
This is something nice and useful done by KIO. Dolphin has it for some time and Krusader now, too.

I politely disagree. It is true that "unmanaged" corresponds to the the current behaviour- just start all jobs right away. But during testing i often had the situation that my system slows down a lot when more than two copy operations are working in parallel. And I believe most users will expect the queue behaviour.

I actually think there are pros and cons to both approaches regarding default behaviour. E.g. for PCs with SSD would arguably be better the unmanaged mode so any slow remote/flashdisk job would not delay fast local jobs. I'm still unsure if I'm right. And Your arguments are definitely valid. Like You said - config option would be good.

I was thinking about this in the past and the best I came up with so far was: automatically deciding whether to queue or launch-in-parallel job based on mountpoint (and hostname of remote location respectively) of the data destination (moving & copying jobs). I'm sure I didn't think this perfectly through. Though in my sunny world this would queue tasks selectively for separate flashdisks, separate remote locations etc. but run these grouped jobs in parallel. What do you think - is this possible or is it just my imagination going off?

New feature: an "undo button"

This is neat, very handy! Thank You for this!


I've finally come up with the patch for the not-focusing refreshed panel: D3394. You were right about the 2 lines:).

abika added a comment.Nov 20 2016, 9:12 PM

More improvements for the job manager:

  • you were right about the "Pause" mode. I just removed it and instead of the clumsy combo box there is now a simple toggle button. (Adding a default configuration setting for the job mode is TODO)
  • I had to add a wrapper class for KIO::Jobs again. But now the queue mode is really working (or better: ready for testing)!

Jobs started with Drag&Drop still not supported :( I have to ask Frameworks/KIO guys again.

abika added a comment.Nov 21 2016, 5:18 PM

Some changes again. Please see commit messages.

About drag&drop: I finally found out that DropJob starts CopyJobs internally and there is no way to access them. I guess I have to patch KJobWidgets myself.


Most of the old functionality is re-implemented now again. If you haven't found any major bugs (me neither) I would like to finally merge back with master. There are already some conflicts and I fear more will arise.

Great work, Alex. Queuing is working perfectly, refreshing is fixed, and we have a nice job overview:). Many thanks for Your work!

I agree with merging in master and then if we find some minor problems, we can fix them there.

I have found one corner case: If I start copying with "Enqueue Operation" checked in dialog, the job starts paused even if it is a first job. Now when I think about it, it can actually be a feature and it can have some use-cases, right? Ok, I like it, nevermind :).

Then I have a little usability problem with the "Enqueue Operation" checkbox itself. Maybe I'm too conservative or I miss something but I feel like it slows down a user when using the queue. One have to click it with mouse or hit tab 2x then hit space and then enter to manually queue a job. On the other hand F2 button was much faster and on par with e.g. Total Commander (I'm not saying this is a requirement though). The F2 button or the checkbox can be disabled when overall Job Queue Mode is enabled. Or it can have opposite meaning = paralellize job. I'll be happy to revert & implement it if You and others agree on the behaviour. Anyway maybe I've got it all wrong. What do You think?

abika added a comment.Nov 23 2016, 7:34 PM

Maybe for clarification:

  • start job with "Enqueue" -> Job is always created in pause mode
  • start job without "Enqueue" but "Queue Mode" is on -> Job is started only if no other job is running
  • start job without "Enqueue" and "Queue Mode" is off -> Job is always started

But I agree the option titles are confusing.

I have found one corner case: If I start copying with "Enqueue Operation" checked in dialog, the job starts paused even if it is a first job.

Yes, the option should be renamed to "Don't start job" or something. I'm unsure.

Then I have a little usability problem with the "Enqueue Operation" checkbox itself. Maybe I'm too conservative or I miss something but I feel like it slows down a user when using the queue. One have to click it with mouse or hit tab 2x then hit space and then enter to manually queue a job. On the other hand F2 button was much faster and on par with e.g. Total Commander (I'm not saying this is a requirement though). The F2 button or the checkbox can be disabled when overall Job Queue Mode is enabled. Or it can have opposite meaning = paralellize job. I'll be happy to revert & implement it if You and others agree on the behaviour. Anyway maybe I've got it all wrong. What do You think?

I thought this is more intuitive. The job is always created - on choosing "F2 Queue Job" or "Ok". The only difference is that the job is always paused or not. It is more like an argument for the operation rather than two distinct operations.

But if you like you can change the dialog to your needs.
Oh, and the new shortcut (instead of F2) is "Alt+Q".


(Hopefully) last change before merge: Show a blocking modal dialog on quit if jobs are not finished yet.

Thanks for clarification! Now I see what the "Enqueue Operation" checkbox is really about. So every job added with the checkbox checked has to be unpaused manually to perform. The checkbox should probably be renamed to "Start Paused" but I'm unsure, too.

I'd still preserve the F2 button, but the queue toggle button in toolbar is awesome and meets my requirements so I'm fine with current state, too. Anyway, I'd like to re-add the F2 button during weekend, if you are OK with that. Since the checkbox has different use cases, I should probably preserve it, right?

Again, awesome job, Alex, thanks a lot! I'm for merging it.

I have one crash report though (sorry, I had no time to look into it yet. I will on weekend):

  1. Start a job
  2. During it exit Krusader. The new dialog about running job is shown.
  3. Hit Cancel in the dialog.
  4. After job is finished (or if it is aborted manually), Krusader crashes.

It actually segfaults with hitting directly the abort dialog button, too.

abika added a comment.Nov 25 2016, 7:58 PM

So every job added with the checkbox checked has to be unpaused manually to perform.

Not necessarily. If queue mode is turned on it will be started automatically once it is the next job in line.

The checkbox should probably be renamed to "Start Paused" but I'm unsure, too.

"Start Paused" is a lot better. Changed it.

I'd still preserve the F2 button, but the queue toggle button in toolbar is awesome and meets my requirements so I'm fine with current state, too. Anyway, I'd like to re-add the F2 button during weekend, if you are OK with that. Since the checkbox has different use cases, I should probably preserve it, right?

Just do what you think is right. I have confidence:) But what is the operation you want to (re-)add with the F2 button? Before, it was adding the job to the queue manager but this is now always done.

I have one crash report though (sorry, I had no time to look into it yet. I will on weekend):

Damn, so obvious. And not tested enough. Fixed now.

abika added a comment.Nov 26 2016, 4:44 PM

Merged with master

102 files changed, 2716 insertions(+), 5014 deletions(-)

yeah

Great work. thanks! Also thanks for fixing the focus in search modes etc.

Anyway I finally came up with proposed diff D3536 for the "F2 Queue" button (sorry it took so long). I didn't commit it yet so it can be tuned/criticized:).

abika added a comment.Dec 6 2016, 2:19 PM

FYI: https://git.reviewboard.kde.org/r/129540/

Operations started with drag&drop will (potentially) be manageable with KF5.30

Very nice, thanks for taking care of the KIO patch!

abika added a comment.Dec 10 2016, 5:07 PM

Yes, obvious mistake. Fixed and thanks for reporting!

I'm still unsure if we should split the vfs class (in e.g. FileLister and FileOperations). Currently there is only one method that is used without vfs: copyFiles().

janlepper added a comment.EditedDec 14 2016, 4:32 AM
In T3419#70758, @abika wrote:

I'm still unsure if we should split the vfs class (in e.g. FileLister and FileOperations). Currently there is only one method that is used without vfs: copyFiles().

It's problybly not that important.

Thinking about it, a more beneficial thing would be moving to a non-blocking
refresh refresh like in KDirLister.

Then there is vfs::calcSpace() which also blocks.
From there I stumbled on vfs::calcSpaceKIO and "TODO called from another thread, creating KIO jobs does not work here".
I recall that KIO jobs move themselves to the main (or gui?) thread, or is my memory wrong here?

twitt added a subscriber: twitt.Dec 14 2016, 3:52 PM

Thanks for the great work and the new features!
I noticed a minor issue with the toolbar. Since I don't use toolbars very often, I tried to move the Job toolbar to the bottom of the window, but after I restarted Krusader it was always at the top again. I noticed the same behavior with the Main toolbar, so it might be due to KXMLGuiWindow.
Maybe it is even possible to add the progress bar and the play/pause button to the status bar like in KMail for those who do not use toolbars?
Another misbehavior I noticed is, when I drag a file to another panel using the shift (or an other) modifier, the file will be copied instead of moved. Since I had the same issue before the branch was merged to the master branch, I guess it is not related to the rework of vfs.

Anyway I am very thankful for the work you did!

abika added a comment.Dec 14 2016, 7:07 PM

I noticed a minor issue with the toolbar. Since I don't use toolbars very often, I tried to move the Job toolbar to the bottom of the window, but after I restarted Krusader it was always at the top again. I noticed the same behavior with the Main toolbar, so it might be due to KXMLGuiWindow.

Toolbar position wasn't saved. Fixed now.

Maybe it is even possible to add the progress bar and the play/pause button to the status bar like in KMail for those who do not use toolbars?

This is troublesome. The statusbar cannot simply contain QActions (afaik) and it will take more horizontal space. It is probably possible but would need some work.

Another misbehavior I noticed is, when I drag a file to another panel using the shift (or an other) modifier, the file will be copied instead of moved. Since I had the same issue before the branch was merged to the master branch, I guess it is not related to the rework of vfs.

Fixed. Thanks for reporting.

Anyway I am very thankful for the work you did!

Thanks!

Thanks for the fast fixes. The toolbar position and the drop action are working just fine. You're right about the statusbar, it is not based on QAction but on QWidgets. Therefor one would need to rewrite JobMenuAction into a JobMenuWidget. But since the toolbar position is at the buttom, it will work for me anyway.
Thanks again!

janlepper added a comment.EditedJan 2 2017, 11:12 PM
In T3419#70758, @abika wrote:

I'm still unsure if we should split the vfs class (in e.g. FileLister and FileOperations). Currently there is only one method that is used without vfs: copyFiles().

It's problybly not that important.

Thinking about it, a more beneficial thing would be moving to a non-blocking
refresh refresh like in KDirLister.

Then there is vfs::calcSpace() which also blocks.
From there I stumbled on vfs::calcSpaceKIO and "TODO called from another thread, creating KIO jobs does not work here".
I recall that KIO jobs move themselves to the main (or gui?) thread, or is my memory wrong here?

Hmm, KIO should be thread safe according to https://git.reviewboard.kde.org/r/118614/.

Edit: Or maybe not really :(
"And that's just one unittest, there could be other kio features to test from threads."

abika added a comment.Jan 3 2017, 8:41 PM

Oh, the job creation in vfs::calcSpaceKIO() is working now! I could swear that there was an Qt error message the last time I tried, saying something about not being able to create an QObject with a parent in a different thread.

However, the busy-waiting-while-loops cannot be used anymore. Well, I will work on it now...

Just so we don't duplicate our efforts: T5017

I've stayed clear of space calculation so far.

abika renamed this task from Rework of VFS to Rework of VFS (Now: FileSystem).Jun 5 2017, 2:58 PM
abika updated the task description. (Show Details)
abika moved this task from In progress to Done on the Krusader board.

Finally mark this done.

I'm always available for comments!

asensi closed this task as Resolved.EditedAug 4 2017, 8:20 PM

Marked as "Closed, Resolved" (it was "Open, Needs Triage"). Thanks for the good work!