Skip to content

Conversation

d2dyno1
Copy link
Member

@d2dyno1 d2dyno1 commented Feb 19, 2021

This PR polishes and fine-tunes Bundles PR. This is the final part that completes Bundles

Things yet to do

Closes #3449
Closes #3270
Closes #3724

@R3voA3
Copy link
Contributor

R3voA3 commented Feb 23, 2021

Would it be in the scope of this PR to add context menu entries to add an item to a bundle?

@crashmit
Copy link

Suggestion to improve bundles further:
image
Currently there is no option to open bundle items in a new pane, if I'm not mistaken. Have you thought about implementing this so far?

@d2dyno1
Copy link
Member Author

d2dyno1 commented Feb 24, 2021

Would it be in the scope of this PR to add context menu entries to add an item to a bundle?

Hmm, this is a bit of work and actually exceedes the scope of this PR. Drag & drop already works with bundles so there's no point for now to add context menu option

@d2dyno1
Copy link
Member Author

d2dyno1 commented Feb 24, 2021

Suggestion to improve bundles further:
image
Currently there is no option to open bundle items in a new pane, if I'm not mistaken. Have you thought about implementing this so far?

Sounds like a good idea. I'll work on that

@R3voA3
Copy link
Contributor

R3voA3 commented Feb 24, 2021

Would it be in the scope of this PR to add context menu entries to add an item to a bundle?

Hmm, this is a bit of work and actually exceedes the scope of this PR. Drag & drop already works with bundles so there's no point for now to add context menu option

Fair enough, although Drag & Drop doesn't work reliably.

@d2dyno1 d2dyno1 mentioned this pull request Mar 4, 2021
@d2dyno1 d2dyno1 marked this pull request as ready for review March 4, 2021 20:01
@d2dyno1 d2dyno1 requested a review from yaira2 March 4, 2021 20:52
yaira2
yaira2 previously approved these changes Mar 4, 2021
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.

Nice work! LGTM

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Mar 4, 2021
@yaira2 yaira2 changed the title Improved Bundles! Made some enhancements to the bundle's widget Mar 4, 2021
#region IDisposable

// TODO: This Dispose() is never called, please implement the functionality to call this function.
// This IDisposable.Dispose() needs to be called to unhook events in BundlesWidget to avoid memory leaks.
Copy link
Member Author

@d2dyno1 d2dyno1 Mar 5, 2021

Choose a reason for hiding this comment

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

This one is important. Currently we have a memory leak in Bundles because IDisposable.Dispose() in YourHome.xaml.cs is not called.

@yaichenbaum fyi

Copy link
Member Author

@d2dyno1 d2dyno1 Mar 5, 2021

Choose a reason for hiding this comment

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

The solution is already in place but YourHome.Dispose() needs to be called which exceeds the scope of this PR :)

@d2dyno1 d2dyno1 changed the title Made some enhancements to the bundle's widget Made some enhancements to the Bundles widget Mar 5, 2021
@yaira2 yaira2 added needs - code review and removed ready to merge Pull requests that are approved and ready to merge labels Mar 7, 2021
@yaira2 yaira2 requested a review from gave92 March 7, 2021 03:16
@gave92
Copy link
Member

gave92 commented Mar 7, 2021

Support for adding shourtcut files

How do you add shortcut files to a bundle since shortcuts can't be dragged?
Edit: you can from explorer

Option to rearrange Bundle items by dragging

Seems like I'm not able to drag items inside bundles, I can only rearrange bundles

@d2dyno1
Copy link
Member Author

d2dyno1 commented Mar 7, 2021

Option to rearrange Bundle items by dragging

Seems like I'm not able to drag items inside bundles, I can only rearrange bundles

It does work, but you must hold the item at the very edge of it. The reason it can't be dragged when directly clicking on it is because the button is clicked instead since it covers the entire gridviewrow area.

buttonInTheWay

@gave92 Do you happen to know a solution to such problem?

@gave92
Copy link
Member

gave92 commented Mar 7, 2021

@gave92 Do you happen to know a solution to such problem?

What about removing the Button and adding IsItemClickEnabled/ItemClick to the GridView?

@d2dyno1
Copy link
Member Author

d2dyno1 commented Mar 7, 2021

@gave92 Do you happen to know a solution to such problem?

What about removing the Button and adding IsItemClickEnabled/ItemClick to the GridView?

How would flyout work with it?
image

@gave92
Copy link
Member

gave92 commented Mar 7, 2021

I'm positive ContextFlyout can be set on any element (e.g a Grid) not just Buttons (or you can switch to FlyoutBase.AttachedFlyout + RightTapped event but there should be no need)

@d2dyno1
Copy link
Member Author

d2dyno1 commented Mar 7, 2021

@gave92 fixed! Thank you! 😊

@d2dyno1 d2dyno1 requested a review from gave92 March 7, 2021 12:45
@gave92 gave92 self-requested a review March 7, 2021 19:49
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Mar 7, 2021
@yaira2 yaira2 merged commit eb4561c into files-community:main Mar 7, 2021
@d2dyno1 d2dyno1 deleted the improved_bundles branch April 5, 2021 17:30
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.

Autofocus on text field Warning about existed bundle name [Feature request] Add shortcut support to Bundles
5 participants