Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Show file hashes in properties like HashTab. #3722 #4300

Closed
wants to merge 12 commits into from
Closed

Show file hashes in properties like HashTab. #3722 #4300

wants to merge 12 commits into from

Conversation

oleitao
Copy link
Contributor

@oleitao oleitao commented Mar 25, 2021

Resolved / Related Issues
This PR is used to implement a new feature to show file hashs MD5/SHA1/CRC32 on Properties panel and compare the current hashs with hashs of another files.

Validation

  • Get folder Properties and get his hashs
  • Compare the current file hashs with other file hashes (check if they are different)
  • Compare the current file hashs with same file hashes (check if they are equals)

Screenshots (optional)
hashs

@yaira2
Copy link
Member

yaira2 commented Mar 25, 2021 •
edited
Loading

@oleitao Do you really think we should include the option to compare with other Files? I think the properties dialog should be limited to the selected file. Also, can you put the hashes section after the details section?

@oleitao
Copy link
Contributor Author

oleitao commented Mar 25, 2021

@oleitao Do you really think we should include the option to compare with other Files? I think the properties dialog should be limited to the selected file. Also, can you put the hashes section after the details section?

@yaichenbaum i saw that suggestion on 3722 but i can remove that. Please let me know.

@yaira2
Copy link
Member

yaira2 commented Mar 25, 2021

@oleitao It's up to you, but in my opinion, if it's working fine, we might as well keep it.

@oleitao
Copy link
Contributor Author

oleitao commented Mar 25, 2021 •
edited
Loading

@oleitao It's up to you, but in my opinion, if it's working fine, we might as well keep it.

I've some suggestions about an previous related PR 4121 which is related with MD5 hash generation.

  • In order to don't have MD5 hash on two placed Properties -> General(tab) and Properties -> Hashes (tab). I suggest to remove MD5 hash field of General tab.

  • Discard that PR 4121

Do you agree?

@yaira2
Copy link
Member

yaira2 commented Mar 25, 2021 •
edited
Loading

Do you agree?

@oleitao I agree, it should only be in one place.

@yaira2 yaira2 requested a review from tsvietOK March 25, 2021 20:44
@yaira2
Copy link
Member

yaira2 commented Mar 26, 2021

@oleitao Should the compare option be a little lower down on the page similar to the screenshot shared in the issue you linked?

@oleitao
Copy link
Contributor Author

oleitao commented Mar 26, 2021

@oleitao Should the compare option be a little lower down on the page similar to the screenshot shared in the issue you linked?

@yaichenbaum i removed the compare option. Do you want to add compare option again?

@yaira2
Copy link
Member

yaira2 commented Mar 26, 2021 •
edited
Loading

@oleitao Should the compare option be a little lower down on the page similar to the screenshot shared in the issue you linked?

@yaichenbaum i removed the compare option. Do you want to add compare option again?

It's good either way.

@yaira2 yaira2 requested a review from gave92 March 30, 2021 01:34
tsvietOK
tsvietOK suggested changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@tsvietOK tsvietOK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolve conflicts, please.

Files/Views/Pages/PropertiesHashes.xaml.cs Outdated Show resolved Hide resolved
Files/Views/Pages/PropertiesHashes.xaml.cs Outdated Show resolved Hide resolved
Files/Interacts/Interaction.cs Outdated Show resolved Hide resolved
Files/Interacts/Interaction.cs Outdated Show resolved Hide resolved
@tsvietOK tsvietOK added changes requested Changes are needed for this pull request and removed needs - code review labels Apr 2, 2021
oleitao added 6 commits April 3, 2021 16:57
@oleitao
Show file hashes in properties like HashTab (fix) files-community#3722
18a944a
@oleitao
fix files-community#3722
a49935d
@oleitao
Merge branch 'main' into file-hashes-properties
b483143 
# Conflicts:
#	Files/Files.csproj
#	Files/Interacts/Interaction.cs
#	Files/ViewModels/Properties/FileProperties.cs
#	Files/ViewModels/SelectedItemsPropertiesViewModel.cs
@oleitao
fix
0cfd716
@oleitao
fix
0f0a82f
@oleitao
fix
154d857
@yaira2 yaira2 requested a review from tsvietOK April 6, 2021 18:48
tsvietOK
tsvietOK suggested changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@tsvietOK tsvietOK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test your changes? Nothing happens when opening Hashes tab (except CRC32 hash)
image
By the way, CRC32 hash should be 04CE0987.

Files/Views/Pages/Properties.xaml Outdated Show resolved Hide resolved
Files/Views/Pages/PropertiesHashes.xaml.cs Outdated Show resolved Hide resolved
Files/Views/Pages/PropertiesHashes.xaml Outdated Show resolved Hide resolved
Files/Views/Pages/PropertiesHashes.xaml Outdated Show resolved Hide resolved
Files/Views/Pages/PropertiesHashes.xaml Outdated Show resolved Hide resolved
Files/Views/Pages/PropertiesHashes.xaml Outdated Show resolved Hide resolved
@oleitao
Copy link
Contributor Author

oleitao commented Apr 7, 2021

By the way, CRC32 hash should be 04CE0987.

I've tested CRC32 and compared the output with Hash Viewer and all CRC32 generated hashes was mathed.

Capture1

Capture2

Capture3

@yaira2 yaira2 requested a review from tsvietOK April 7, 2021 21:26
tsvietOK
tsvietOK suggested changes Apr 10, 2021
View reviewed changes
Copy link
Contributor

@tsvietOK tsvietOK left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The calculating progresses of SHA1 and CRC32 hashes are not displayed.
  2. Wrong CRC32 hash.
    image
    image
  3. The SHA1 hash is truncated.
  4. Sometimes it is blocking UI thread.
  5. How to compare Text hash with calculated hash?
  6. Sometimes when canceling calculation crc32 hash app crashes.
    Please, test it properly before submitting for review.

@yaira2
Copy link
Member

yaira2 commented Apr 18, 2021

@oleitao Can you address the requested changes?

@oleitao
Copy link
Contributor Author

oleitao commented Apr 21, 2021

@oleitao Can you address the requested changes?

It's better not address this PR to me.

@yaira2 yaira2 closed this Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsvietOK tsvietOK tsvietOK requested changes

@gave92 gave92 Awaiting requested review from gave92

Assignees
No one assigned
Labels
changes requested Changes are needed for this pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oleitao @yaira2 @tsvietOK