Skip to content

Conversation

VigneshVoid
Copy link
Contributor

I have added support for two new properties similar to Windows Explorer. We can have a whole lot of properties from the list in the URL

https://docs.microsoft.com/en-us/windows/win32/properties/core-bumper

Screenshot

@ghost ghost added the needs - code review label Jun 14, 2020
@yaira2
Copy link
Member

yaira2 commented Jun 14, 2020

Instead of getting the info from code behind in the properties dialog, we should do it in SelectedItemsPropertiesViewModel and bind the TextBlock in the properties dialog to the properties in the SelectedItemsPropertiesViewModel.

@VigneshVoid
Copy link
Contributor Author

Sure will work on that and add in the next commit @yaichenbaum

@yaira2 yaira2 requested review from tsvietOK and yaira2 June 14, 2020 05:22
@yaira2
Copy link
Member

yaira2 commented Jun 14, 2020

Is there a reason you added .Properties? I feel that is a little redundant in this case since SelectedItemsPropertiesViewModel is going to be limited to the file properties anyways.

@VigneshVoid
Copy link
Contributor Author

I added to separate Main Item Properties from any visibility properties or any local properties need for the methods within ViewModel. So it will be easy to differentiate from Main and Local Method Properties.

Mainly for Code Readability. @yaichenbaum

@tsvietOK
Copy link
Contributor

I don't think it is really useful to display item owner in Properties dialog. @yaichenbaum What do you think about it?
Moreover, you displaying "File owner" even if item is folder.
However properties retrieving way you offered could be useful in the future.

@yaira2
Copy link
Member

yaira2 commented Jun 16, 2020

@tsvietOK I agree that it's a bit much, what we can do is add a feature flag for this to the experimental page and if a large percentage of users turn it on we can look into making this the default.

@yaira2
Copy link
Member

yaira2 commented Jun 16, 2020

@xpoppyx At some point we can do that, but I think we should get the basics first, for example, showing multiple dialogs if the user clicked properties when multiple files are selected.

@VigneshVoid
Copy link
Contributor Author

VigneshVoid commented Jun 17, 2020

"showing multiple dialogs if the user clicked properties when multiple files are selected."

@yaichenbaum supported the above requirement in this commit

Screenshot

@yaira2
Copy link
Member

yaira2 commented Jun 17, 2020

@VoidExploiter I actually made a mistake in the above comment, I should have said support for properties when opening the dialog when multiple items are selected. We can definitely explore keeping this but my thoughts are that when users select multiple files and hit "properties", they will want to know the size of all of them together and other information once it's extended.

@VigneshVoid
Copy link
Contributor Author

We can provide a settings to open properties in individual windows for multiple files or on same window.

I will work on that and also opening a window with a combined properties.

image

@yaira2
Copy link
Member

yaira2 commented Jun 17, 2020

@VoidExploiter Can you add the setting to the experimental settings page? This pull request is looking very good so far 🙂.

@VigneshVoid
Copy link
Contributor Author

Sure will add multiple windows and also file owner flag in that

@VigneshVoid
Copy link
Contributor Author

VigneshVoid commented Jun 17, 2020

@yaichenbaum Moved File Owner and Multiple windows to Experimental Flag

image

@yaira2
Copy link
Member

yaira2 commented Jun 17, 2020

@VoidExploiter Can you add strings for the new text that you added?

@yaira2
Copy link
Member

yaira2 commented Jun 17, 2020

I added to separate Main Item Properties from any visibility properties or any local properties need for the methods within ViewModel. So it will be easy to differentiate from Main and Local Method Properties.

Mainly for Code Readability. @yaichenbaum

I don't think this is necessary, as anything this ViewModel is already for item properties.

@VigneshVoid
Copy link
Contributor Author

Okay will add the strings and move properties Model back into ViewModel

@VigneshVoid
Copy link
Contributor Author

I added to separate Main Item Properties from any visibility properties or any local properties need for the methods within ViewModel. So it will be easy to differentiate from Main and Local Method Properties.
Mainly for Code Readability. @yaichenbaum

I don't think this is necessary, as anything this ViewModel is already for item properties.

Removed in this commit

@VigneshVoid
Copy link
Contributor Author

VigneshVoid commented Jun 18, 2020

@VoidExploiter Can you add strings for the new text that you added?

Added for English here. Do I need to add for other languages too?

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Please address the above comments.

@yaira2 yaira2 added changes requested Changes are needed for this pull request and removed needs - code review labels Jun 18, 2020
VigneshVoid and others added 3 commits June 18, 2020 11:28
Co-authored-by: Yair Aichenbaum <39923744+yaichenbaum@users.noreply.github.com>
Co-authored-by: Yair Aichenbaum <39923744+yaichenbaum@users.noreply.github.com>
@VigneshVoid VigneshVoid requested a review from yaira2 June 18, 2020 06:54
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed changes requested Changes are needed for this pull request labels Jun 18, 2020
@yaira2 yaira2 merged commit f6a3c01 into files-community:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants