jun7 / rox-filer

ROX file manager
24 stars 6 forks source link

Xattr Browser: Allow removal of un-editable user attributes. #170

Closed woodenshoe-wi closed 6 years ago

woodenshoe-wi commented 6 years ago

Some programs may set xattr values that are not valid strings and therefor un-editable. These should still be removable.

jun7 commented 6 years ago

Thank you!

woodenshoe-wi commented 6 years ago

I have more changes but I had to sort out merge conflicts first.

I haven't thoroughly tested this after fixing the merge conflicts but it seems to work fine in quick tests, action.c: Fix "overwrite if newer" check-box Do you want me to issue a pull request for it?

jun7 commented 6 years ago

Only -> Always makes sense. The Newer check box seems works without the fix. What is the problem? Also is not the force option for deletion?

woodenshoe-wi commented 6 years ago

There are several different versions of rox-filer in use and I had been happy using an older one that would prompt before overwriting any file whether it was older or newer. Some users on the Puppy Linux forum preferred a newer version of rox-filer that would overwrite files without prompting (this could have been before the "force" box was introduced) but it would not make a difference if the overwritten files were older or newer.

I was trying to make the "Newer" check box overwrite older files without prompting but still prompt before overwriting newer files.

Some forum members seemed very worried about accidentally overwriting newer files with older ones, so I reverted the commit that changed the meaning of "Force" to "always overwrite" to go back to the original meaning (whatever that was... seemed to be related to deleting read-only files without prompting?).

I wanted to ask if you wanted the commit before I issued a pull request because it does change the user interface and is not strictly a bug fix.

I am working on a sort of "classic" fork of rox-filer that will compile on old operating systems and mostly keeps the original user interface, that is where I cherry-picked the commits from. If you want to make changes that's OK, but you are probably in a better position to know what you want to change than I am. I don't want you to feel under any obligation to include these changes, I just wanted to let you know about them in case you were interested in them or any part of them.

jun7 commented 6 years ago

I get it. So the fix means There is no case to over write newer files by old files. Thus using the force only to ignore prompting of merge is bad idea. Doesn't it? BTW even not quiet, when over writing older files with o_newer, prompting are skipped. Is this a bug? if a bug we have not to use the buffer is_newer, just move the 'Newer keep going''s conditional statement to the ignore_quiet flag.

Hmm, but is using mtime for dirs reasonable? the mtime of dirs is not recursive. Are not the mtimes of dirs useless?

Just adding merge check box could be a solution? Hmm. Ah you are doing it! Also the 'ignore older' seems good too. Then so, I'm happy only if you left the force option because thinking of that flags annoys us ;P There are cases want to say just do it.

woodenshoe-wi commented 6 years ago

Here is a second attempt, action.c: Change "overwrite if newer" check-box It is past 1:30 in the morning here so you better double check my work.

I had been working with a combination of the dtomas fork and the seirios fork when I first wrote the fix, and to fix the conflicts when cherry-picking I reverted ebacaf1 but I guess that commit had already fixed the problem except that if you want to overwrite files with an identical mtime (which are probably identical copies) when the "Newer" check box is ticked the comparison has to be >= instead of > info.st_mtime >= dest_info.st_mtime

BTW even not quiet, when over writing older files with o_newer, prompting are skipped. Is this a bug?

I think it was an old bug that I either didn't notice or couldn't figure out if it was necessary. I tried to fix it but you will have to check for any unintended consequences.

Hmm, but is using mtime for dirs reasonable? the mtime of dirs is not recursive. Are not the mtimes of dirs useless?

Could you give me a line number? I am not sure what part of the code you are talking about.

Just adding merge check box could be a solution? Hmm. Ah you are doing it! Also the 'ignore older' seems good too. Then so, I'm happy only if you left the force option because thinking of that flags annoys us ;P There are cases want to say just do it.

Although I was able to cherry-pick the first commit and fix the merge conflicts, the commit that adds the 'merge' and 'ignore older' check boxes depends on action.c: Make do_move2 work more like do_copy2 and I am having trouble fixing the merge conflicts caused by the 'Add num' changes. Maybe it's not a big problem, but it is too late at night for me to figure out.

jun7 commented 6 years ago

Could you give me a line number?

1373 rox doesn't use st_mtime for dirs (!merge). And I agree this choice. Thus I think adding merge option is good. But hmm have you noticed the rox's file copy is bloody slow than others? So I'm negative to change move to slow too.

woodenshoe-wi commented 6 years ago

Ok, now I understand the problem with mtime. Does f6a91cd fix the logic error? I haven't tested it but I think it is easier to explain as code.

But hmm have you noticed the rox's file copy is bloody slow than others?

I don't know compared to other file managers, but if you are comparing to move is move still faster when moving files to another file system? Moving files inside the same file system is always faster than copying.

If you don't want the "merge" check box added to the "move" dialogue that would remove the dependency on 44ade55 and would be easier than fixing the conflicts 😁

jun7 commented 6 years ago

Ok, now I understand the problem with mtime. Does f6a91cd fix the logic error? I >haven't tested it but I think it is easier to explain as code.

Yes, is_newer variable is not necessary because it used only once though.

same file system is always faster than copying

The cause is not that, just because the fork_exec_wait is slow I think.

Hmm, but looks like the 'merge' is the only solution. Since you are working hard around copy and move, don't you interest to fix it too?

woodenshoe-wi commented 6 years ago

Ok, I got rid of the is_newer variable and I do have to agree that it is a lot easier to understand now 5157299

Hmm, but looks like the 'merge' is the only solution. Since you are working hard around copy and move, don't you interest to fix it too?

If you mean trying to make copying faster, no I am not interested in doing that kind of work.

You have already given me some helpful insight into how the do_copy2 function is supposed to work and I am interested in trying to make the do_move2 function work in a manner consistent with the do_copy2 function. I want to make sure that the sequence numbering is working the way you want before I try to change do_move2, maybe I have to add the 'merge' and 'ignore older' check boxes only to the "copy" dialogue for testing and add them to the "move" dialogue later.

Much of what I collected in my fork of rox-filer was from the dtomas and seirios branches, various patches written by Puppy Linux forum members and some feature requests and bug fixes that I wrote. I wanted to return the favor by offering my bug fixes to the dtomas and seirios branches for the code I took from there, and then work on cherry-picking anything you wanted. There is no need for you to accept any changes that you do not like, I just want to know what I should work on cherry-picking and do it one piece at a time.

jun7 commented 6 years ago

If you mean trying to make copying faster, no I am not interested in doing that kind of work.

I tried to change it fast and it works well that moving 10000 files takes only 5 secs by use of g_file_move. It is same as pcmanfm. So now I'm very positive to change do_move2 to like do_copy2.

woodenshoe-wi commented 6 years ago

I would gladly start working on top of your changes but I can't find them.

I checked the stable, temp and test branches too.

Maybe a temporary branch is needed so we can both work on the changes? (I would issue pull requests to this branch instead of master)

jun7 commented 6 years ago

now it is in test branch. Sorry it is not done yet.

woodenshoe-wi commented 6 years ago

No problem, there is no hurry, but I thought you said copying was slow?

The changes I found in the test branch are to do_move2

jun7 commented 6 years ago

Yes copy is slow because of fork_exec_wait. Move is slow too but it is not recursive until your works. I thought before changing 'move' to recursive as 'copy', we have to get rid the slow.

Ah, but I have misunderstood your commit. I thought it is recursive but actually it only recursive on exists cases. Sorry, the commit was not necessary. The g_file_move is just another optimization now, Not relates the changing 'move'. So you can ignore the test branch.

woodenshoe-wi commented 6 years ago

I cherry-picked Add "Ignore Older" and "Merge" checkboxes

I did not add a "Merge" checkbox to the "Move" dialog because I think there have been too many changes to do_copy2 to use the old "action.c: Make do_move2 work more like do_copy2" commit. I will have to start over the same way I did the first time by copying the do_copy2 function to a text file and copying the do_move2 function to a text file and running diffs while I edit do_move2.

I want to wait until do_copy2 is in it's final version before I start editing do_move2.

Can you test if the sequence numbering works the way you want with various combinations of "Newer", "Ignore Older" and "Merge"?

I do not know if you want the sequence numbering to override the "Newer", "Ignore Older" and "Merge" options, or to interact to choose which files should get sequence numbers.

jun7 commented 6 years ago

Can you test if the sequence numbering works the way you want with various >combinations of "Newer", "Ignore Older" and "Merge"?

Yes of course.

I do not know if you want the sequence numbering to override the "Newer", >"Ignore Older" and "Merge" options, or to interact to choose which files should >get sequence numbers.

The seq no doesn't override other options. It currently having a bug though.

woodenshoe-wi commented 6 years ago

Does this fix the bug? action.c: Fix sequence numbering and "Ignore Older"

jun7 commented 6 years ago

Yes, it seems fixed.

woodenshoe-wi commented 6 years ago

OK, I think I finished the last commit.
action.c: Make do_move2 work more like do_copy2 - added merge box

I added a comment to the commit with the diff between the do_copy2 and do_move2 functions. It is at the bottom of the web page.

woodenshoe-wi commented 6 years ago

Do you think any more changes are needed or is it Ok to issue a pull request?

jun7 commented 6 years ago

It is OK. Sorry for the delay.