Skip to content

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
@yaira2 yaira2 added the ready for review Pull requests that are ready for review label 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! :)

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.

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Jan 7, 2025
@yaira2 yaira2 merged commit bc7e862 into files-community:main Jan 7, 2025
6 checks passed
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.

Bug: Soft link file details layout information misaligned

3 participants