Grey out files that have been Ctrl-X'ed #88
Conversation
|
It's great that you work on it! I'll read your code as soos as I find some free time. Thanks! |
|
I use pcmanfm-qt, I do it for myself and everyone else. It's hobby. But also please be patient when waiting for my responses and changes, could take some time too. I'm not that content with this work but I couldn't find better solution actually.
More detailed discussion when ready.. |
src/foldermodel.cpp
Outdated
| case FileIsCutRole: | ||
| bool isCut = false; | ||
| if (folder_ && folder_->hasCutFiles()) { // most of the time false | ||
| isCut = item->isCut() || item->info->isCut(); // then dynamic evaluation |
There was a problem hiding this comment.
Why is spread twice (in FileInfo and in FolderModelItem)? Because of const-ness of FileInfo. Could be casted away but should be?
|
Anyway, I finished it the way it was started. I forced pushed it as a full new patch but nothing has changed from before (if you started to review already), things were only added. If settled and approved, I'll make another small PR in pcmanfm-qt repo to finalize it and make actual CTRL-X to work. (Surely as mentioned before, this was not that straightforward as it may have seemed) |
You could do so now for people to be able to test your idea. In your pcmanfm-qt PR's comment, you could add "follows #88" or "based on ...". |
|
BTW, sorry that I still haven't had time to read this! It needs to be read carefully because you've changed sensitive parts of the code. I'll find some time soon. |
|
Before going to bed (it's past midnight here), I took a look at the code and hoped that it could be made simpler. However, graying out cut files isn't as easy as it might seem and you've done a great job here. Including Anyhow, there's a serious issue, IMHO. In your code, I couldn't find a mechanism for removing the grayed-out state when another file is copied. So, I compiled your branch and saw the problem. Since the grayed-out state is for the user to know which file will be cut and pasted the next time, if that state isn't removed after copying another file, it'll be misleading. I don't mean that the implementation of this property is easy but it's necessary. Please keep up the good work! Thanks! |
|
Oh! More importantly, we need a mechanism to watch the clipboard! Once something is copied/cut anywhere, the grayed-out state should be removed. Whether we need another approach or this one could be modified, I don't know yet. |
|
Your last commit isn't enough. The grayed-out state should be removed on copying anywhere -- copying a text inside a text editor, for example. |
|
Yes, now I understand the problem. I can only hope I find a solution which wouldn't touch the core of the lib too much. Until now it was mostly about adding things. But this needs to be figured out, UI cannot mislead about its states, that would be a bug anyway. (PR in pcmanfm-qt has to wait, this is not test ready yet again) |
I agree. I hope you'll find a solution for copy/cut in other apps. |
|
My poor 2 cents.... I think we should listen to to |
Yes, this is the way I think, same approach is implemented in Dolphin file manager. I think I can come up with something but need some time.
To avoid adding isCut state to |
I don't think @palinek meant that. Keeping track of files that are cut is at the core of your approach and since the directory may change, it seems necessary. We all agree that QClipboard should be watched. |
|
@vc-01 I'm still suspicious about graying out files on such a deep level as that of |
|
Two small bits to review first (bug-fixes). Then the main patch, it's done with the least possible changes to the lib. In the next iteration, maybe I'd like a new global Clipboard (singleton) class which will wrap utilities' clipboard functions and save parsed data. But since this touches the core of the lib a lot, I left it to some subsequent PR. Anyway, cut files are processed differently (aka more locally) for in-app made clipboard changes - same approach as already presented was kept. I kept it, because I somehow believe in how was it done, because dealing directly with selections is fast. More general approach of listening to clipboard, signalling changes, and processing its data by saving and refreshing found folder items, may be cleaner design but slower (speed optimized code is always less readable and understandable). Some slow method Last thing, when pcmanfm-qt is started it doesn't process clipboard data (so doesn't possibly show cut files) because according to QClipboard documentation (tested with Dolphin file manager) (I can do commits squashing for cleaner history at the end before merge) |
|
Requested @PCMan's review. |
src/core/fileinfojob.cpp
Outdated
| if(hasCutFiles && cutFilesHashSet_->count(fileInfo->path().hash())) | ||
| fileInfo->bindCutFiles(cutFilesHashSet_); | ||
|
|
||
| auto fileInfoConst = std::const_pointer_cast<const FileInfo>(fileInfo); |
There was a problem hiding this comment.
I see now, maybe I should have created shared_ptr only here, then const_pointer_cast can be ommited - Can fix..
src/core/fileinfojob.cpp
Outdated
| auto fileInfoConst = std::const_pointer_cast<const FileInfo>(fileInfo); | ||
|
|
||
| results_.push_back(fileInfoConst); | ||
| Q_EMIT gotInfo(path, fileInfoConst); |
There was a problem hiding this comment.
Just to point out. Signal gotInfo is never used in the project.
|
I'm thinking about what is best for this pull request now, after this new merge conflict appeared. History of this branch is getting harder to follow and it's not merged yet, so now I think it's good time to tidy up this - do rebase and squash, which will though invalidate comment history. I will still keep two bugfixes made as separate commits because they are separate things, only they were found during the development of this feature. Stop me if not to do this or if it's better to close this pull request, do rebase + squash, and re-open it as a new one (still learning the github and opensource way). |
|
@vc-01 |
|
I'll try to focus on this starting from tomorrow if no important bug is found. |
|
@vc-01 I was impressed by seeing that if I cut a file in Dolphin, it'll be grayed out in pcmanfm-qt too and conversely but there's a (small) problem: When a cut file is grayed out in pcmanfm-qt, if something is copied elsewhere, the grayed-out state won't be removed until the cursor goes over the view or the pcmanfm-qt window is focused. This doesn't happen with Dolphin and is an issue. Could you fix it, please? |
|
The problem only happens when desktop is shown (in the desktop mode). |
|
@palinek I'm rethinking variables naming. I think it's not an issue if user in some less common way cut files - like in search results and then files aren't grayed out when browsing back directories. Especially when they aren't gray out in search result. Confusing may be variable naming, because it looks like it's some low level cut files stuff - but it's not, it's used only for visual purposes, sometimes even skipped (hashes search for more than 450 cut files). But if agreed upon, multi directory support is doable (will pollute the code a bit though). EDIT: meant source code variable naming |
Could you explain how it might be possible to do multi-dir cut in pcmanfm-qt (since we don't gray out files when the cut is done by another app or in the search view)? |
e.g. if you have files |
I know (and had edited my last comment) but we don't gray out files when they're cut n the search view. |
|
Actually, the search view has several problems because searching is somehow "enforced". If, at some point, it's fixed, we could think about multi-dir gray-out. And as you said, it's possible, |
I see now edited comments are only visible after F5 / Refresh. As I said above I don't see this as a problem, because (1) if it's working - which means file are cut in clipboard, (2) if they aren't grayed out in search result, this shouldn't appear as bug to user - or at least it wouldn't to me. |
|
OK. BTW, an explanation about |
src/core/dirlistjob.cpp
Outdated
| @@ -62,6 +63,12 @@ void DirListJob::exec() { | |||
| false | |||
| }; | |||
| if(enu) { | |||
| bool bindCutFiles = false; | |||
| if(cutFilesHashSet_ | |||
| && cutFilesHashSet_->size() < 450 /* give up if more than 450 cut files */) { | |||
There was a problem hiding this comment.
@vc-01 Could you please remove the condition about 450 files here and at FileInfoJob::exec()? It isn't needed anymore (there's no problem with thousands of files); moreover, it's confusing to the user.
There was a problem hiding this comment.
It looks like it's OK to remove it, it's fast for >1000 files, but be aware it's very different code path when selecting & cutting files inside a directory and go to a directory with already selected & cut files - hash set search is done only in the latter case.
There was a problem hiding this comment.
but be aware it's very different code path when selecting & cutting files inside a directory and go to a directory with already selected & cut files
I know but don't see any extra CPU usage in the latter case with ~4000 cut files. Do you see any?
There was a problem hiding this comment.
By the way, what about adding !cutFilesHashSet_->empty()? Is it needed?
There was a problem hiding this comment.
I don't see any extra CPU usage in the latter case
I searched for what's the best implementation for this particular case and std::set was a winner, probably it's very capable with even much bigger numbers - but not tested. I will remove the condition.
what about adding !cutFilesHashSet_->empty()? Is it needed?
It's tested already in folder.cpp:693 and folder.cpp:258 with Folder::hasCutFiles()
|
I agree. It's simple and, at the same time, tricky, and especially this part in if(cutFilesHashSet_ && !cutFilesHashSet_->empty()) {
oldCutFilesDirPath_ = cutFilesDirPath_;
} |
|
Yes, Unimportant suggestions:
This comment may also need to be changed: To something like this: |
|
I haven't encountered any problem in 2 days. Tomorrow, I'll test with remote files. |
There was a problem hiding this comment.
Was able to do the last test. My conclusion:
On the one hand, this patch adds a nice feature to libfm-qt/pcmanfm-qt, is fast/light, correctly shows files that are cut by the current app, and doesn't interfere with anything. On the other hand, I couldn't find a better way of doing the same thing. If also a fast way is found for graying out files the are cut by other apps, we could improve it later. So,
GTM.
@agaida Would you please also test and merge it?
I've encountered with such naming when functions do something more which should be mentioned in their names. As you can see I've had problem with this name too, cause I changed it twice.
Will change.
Please let me do the squashing first, this branch history is unneeded to keep IMO. |
|
@vc-01 When it comes to names, tastes differ. That was the reason why I said those suggestions were unimportant.
Do so if you see it better in that way but I don't think it's important either. To me, the important thing is the nice job you've done here and it works without problem as far as I tested/used it. |
|
@tsujan I change function names a lot during development and I'm not "sure" with its names often, that's why I'm very open to change it to whatever name. Only its first name confused me a bit when tried to understand its behavior. Squashing: First commit message went a bit obsolete and it's unnecessary for anyone to follow development path of this branch in the future IMO. |
Frankly, no good name came to my mind because it checks something and invalidates it if found. If I hear of a better name in my dreams, I'll tell you ;) Fortunately, a name can't be an obstacle. |
|
Just for the record: This or #112, whichever is merged first, the other one might need a rebase. |
Used `QFileInfo::exists()` instead of `ProxyFolderModel::indexFromPath()` in one case. Also removed the condition about text entry having focus because it wasn't reliable.
|
@vc-01 |
* Made with big help from Tsu Jan * Add visual feedback for files that have been 'cut' to clipboard. This solution is based mostly on weak pointers serving as "observers" to current cut selection. Watching for clipboard changes to set or discard set of cut files. Without support for graying out multi-directory cut selections or files cut in external applications.
|
@tsujan: Good to merge? |
|
@agaida Yes, it's good. Please merge it! |
|
Did it myself. @vc-01 Thanks again for the good work! |
Add visual feedback for files that have been 'cut' to clipboard.
Hello. Referring to issue #503.
I was looking on that and made some working draft. But before more work to be put into this I'd like to know if you think this is good way at all and if you are interested in it.
I put more details in source comments temporarily.
I can answer design decisions but mostly they were influenced by speed requirements (to the point I understood them as such).
This solution is based mostly on weak pointers serving as "observers" to current cut selection, and if present - adjusting icons transparency (icons/thumbnails view only now)
In this approach, two lines of code are necessary to add to pcmanfm-qt because actual CTRL-X key shortcut is defined and processed there - test it with mouse context menu -> "cut" for now.
If you see something very mysterious there, that's possible, this is my first meet with all this, smart pointers etc.
This is not a ready to commit PR yet but a designing proposal.