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.

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

Fix renaming files with long names in GridViewBrowser #898

Merged
merged 4 commits into from
Jun 5, 2020

Conversation

gave92
Copy link
Member

@gave92 gave92 commented Jun 2, 2020

This PR fixes the issue with renaming files with long names when in GridViewBrowser. I'm using ListView + WrapPanel from windows community toolkit from instead of GridView.
Fixes #657

@gave92 gave92 marked this pull request as ready for review June 2, 2020 23:11
@yaira2 yaira2 requested a review from tsvietOK June 2, 2020 23:27
@tsvietOK
Copy link
Contributor

tsvietOK commented Jun 2, 2020

@yaichenbaum This PR changes selection view, is it acceptable?
image
Edit: Button reveal effect is disabled
Edit: I like new folder icon margin

@gave92
Copy link
Member Author

gave92 commented Jun 2, 2020

@yaichenbaum I'm having second thoughts on this: do you know if WrapPanel is virtualized? If it's not performance might be bad on large folders

@yaira2
Copy link
Member

yaira2 commented Jun 2, 2020

@gave92 Virtualization is very important, what if we built this off items repeater?

@gave92
Copy link
Member Author

gave92 commented Jun 2, 2020

I've read that in the latest version of Community Toolkit they rebuilt the WrapPanel to use items repeaters, but I'm not sure if that version is out yet. See here.

Edit: it's in pre-release in version 6.1

Edit2: i'm taking this back as the solution is not viable due to WrapPanel not being virtualized

@gave92 gave92 marked this pull request as draft June 3, 2020 00:31
@gave92 gave92 marked this pull request as ready for review June 5, 2020 11:51
@gave92
Copy link
Member Author

gave92 commented Jun 5, 2020

Ok I approached this differently.

Solution: just use simple a popup with the TextBox
Pros: little code changes, nice "overlay" appearance over the next line items
Todo: perhaps add the "x:Load=False" attribute to the popup? done

@yaichenbaum I'm not using ItemsRepeater as you loose many features GridView gives you (e.g. Selection)

@gave92 gave92 requested a review from yaira2 June 5, 2020 12:12
@ghost ghost added the needs - code review label Jun 5, 2020
@gave92 gave92 requested a review from tsvietOK June 5, 2020 15:13
Added x:Load attribute to edit popup
@yaira2
Copy link
Member

yaira2 commented Jun 5, 2020

Can you share a screenshot of the new behavior?

@gave92
Copy link
Member Author

gave92 commented Jun 5, 2020

image

@yaira2
Copy link
Member

yaira2 commented Jun 5, 2020

Looks good, does the reveal affect still work?

@gave92
Copy link
Member Author

gave92 commented Jun 5, 2020

The revel effect between the files/folders? Yes, I didn't have the mouse on the list in the screenshot

@yaira2 yaira2 requested a review from lukeblevins June 5, 2020 16:06
@yaira2
Copy link
Member

yaira2 commented Jun 5, 2020

I found a bug

  • Start renaming a file
  • Type in a new name
  • Cancel the rename operation
  • Start a rename operation again and the text from last time is still there.

@gave92
Copy link
Member Author

gave92 commented Jun 5, 2020

The current app does the same thing. Can you confirm?
Btw should be easy to fix, should I do it in this PR?

@yaira2
Copy link
Member

yaira2 commented Jun 5, 2020

@gave92 It can be done in a different PR.

@yaira2
Copy link
Member

yaira2 commented Jun 5, 2020

@gave92 Can you resolve the merge conflict?

@yaira2 yaira2 merged commit 92d8d7e into files-community:master Jun 5, 2020
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Jun 5, 2020
@gave92 gave92 deleted the rename_gridviewbrowser branch June 13, 2020 13:20
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