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: Fixed issue where soft link details were misaligned in git repos #16593

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ashrafmansuri
Copy link
Contributor

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Opened Files ...
  2. Create a link in git repo.
  3. Change to detail view.
  4. Check the alignment.

@ashrafmansuri
Copy link
Contributor Author

ashrafmansuri commented Dec 10, 2024

I have created 2 interfaces and substituted in place of GitItem, fixes the alignment issue but unable to get the icon property. Need some help to figure it out.

@yaira2 yaira2 changed the title Bug: Soft link file details layout information misaligned #15998 Fix: Fixed issue where soft link details were misaligned in git repos Dec 10, 2024
@0x5bfa
Copy link
Member

0x5bfa commented Dec 11, 2024

Thank you for your PR!

We use if (item is ShortcutItem) {} a lot so it's important to inherit from ShortcutItem class:

public sealed class GitShortcutItem : ShortcutItem, GitItem {}

Okay, should I discard the previous changes?

Just like this:

if (isSymLink)
{
    if (isGitRepo) { return new() { ... }; }
    else { return new() { ... }; }
}

@ashrafmansuri
Copy link
Contributor Author

Thank you for your PR!

We use if (item is ShortcutItem) {} a lot so it's important to inherit from ShortcutItem class:

public sealed class GitShortcutItem : ShortcutItem, GitItem {}

To inherit from ShortcutItem and GitItem I'll have to remove the sealed keyword. This is the reason I created interfaces.

Okay, should I discard the previous changes?

Just like this:

if (isSymLink)

{

    if (isGitRepo) { return new() { ... }; }

    else { return new() { ... }; }

}

@0x5bfa
Copy link
Member

0x5bfa commented Dec 11, 2024

Seal was added for IL compiler performance aspects, you can remove any time you want.

@ashrafmansuri
Copy link
Contributor Author

Seal was added for IL compiler performance aspects, you can remove any time you want.

Okay. Understood, thank you. Will do today.

@yaira2 yaira2 requested a review from 0x5bfa December 15, 2024 03:02
@0x5bfa
Copy link
Member

0x5bfa commented Dec 15, 2024

You don't need interfaces you introduced.

@ashrafmansuri
Copy link
Contributor Author

You don't need interfaces you introduced.

I won't be able to inherit ShortcutItem and GitItem together. That's why I used interfaces.

@0x5bfa
Copy link
Member

0x5bfa commented Dec 15, 2024

Ahh you're absolutely right, I forgot that.

Can you change "is/as ShortcutItem" to IShortcutItem in everywhere as well in that case? Then you dint have to inherit ShortcutItem and GitItem but should inherit these two interfaces.

@ashrafmansuri
Copy link
Contributor Author

Ahh you're absolutely right, I forgot that.

Can you change "is/as ShortcutItem" to IShortcutItem in everywhere as well in that case? Then you dint have to inherit ShortcutItem and GitItem but should inherit these two interfaces.

Sure thing I'll carefully substitute IShortcutItem in the place of ShortcutItem.

@ashrafmansuri
Copy link
Contributor Author

ashrafmansuri commented Dec 15, 2024

I have replaced all "Is Shortcut" Item with interface so that it checks for the interface properties, I have not changes "as ShortcutItem" because it might want to actually create an Instance of the class rather than the Interface as the ShortcutItem is being derived from ListedItem class also.

@ashrafmansuri ashrafmansuri requested a review from yaira2 December 17, 2024 15:08
@ashrafmansuri
Copy link
Contributor Author

Ahh you're absolutely right, I forgot that.

Can you change "is/as ShortcutItem" to IShortcutItem in everywhere as well in that case? Then you dint have to inherit ShortcutItem and GitItem but should inherit these two interfaces.

I've made the commits! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Soft link file details layout information misaligned
3 participants