Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Bug] Aspect settings not working on MacOS (4.3-pre) #7949

Open
DennisWelu opened this issue Oct 11, 2019 · 12 comments
Open

[Bug] Aspect settings not working on MacOS (4.3-pre) #7949

DennisWelu opened this issue Oct 11, 2019 · 12 comments
Labels
e/2 🕑 2 help wanted We welcome community contributions to any issue, but these might be a good place to start! p/macOS t/bug 🐛 up-for-grabs We welcome community contributions to any issue, but these might be a good place to start!

Comments

@DennisWelu
Copy link

DennisWelu commented Oct 11, 2019

Description

Only 1 of the 3 aspect settings works on XF Mac.

Steps to Reproduce

  1. Create a Xamarin Forms project from template
  2. Add the Mac project to it as per document: https://docs.microsoft.com/en-us/xamarin/xamarin-forms/platform/other/mac
  3. Add image controls to the main page with each aspect setting being used.
  4. Run the app.

Expected Behavior

The expected behavior is demonstrated on iOS:
ImageAspectiOS

Actual Behavior

The actual behavior in a MacOS window:
ImageAspectMac

Basic Information

  • Version with issue: 4.3-pre3
  • Last known good version: ?, but this appears to be a recent major refactor that would be my guess at when it broke: ce148bf#diff-59032a35e10fb51bc88a456719271ac1R1-R12. I think it may have lost some things (see my comment at the end of that link).
  • IDE: VS on Mac

Reproduction Link

ImageAspect.zip

@DennisWelu DennisWelu added s/unverified New report that has yet to be verified t/bug 🐛 labels Oct 11, 2019
@pauldipietro pauldipietro added this to New in Triage Oct 11, 2019
@jfversluis
Copy link
Member

jfversluis commented Oct 11, 2019

Hi @DennisWelu thanks for reporting this. I am kind of confused though.

The title states Mac OS, but you are showing iOS screenshots?

That was to show the expected output, gotcha! :)

@DennisWelu
Copy link
Author

👍 @jfversluis Yup! I should have captioned them, but the first image (expected) is actually the iOS simulator running the same code. The 2nd image (actual) is a MacOS window just shaped roughly the same aspect so it's a closer apple to apples comparison.

@jfversluis
Copy link
Member

I'm wondering if this ever worked, even before the refactor you're mentioning.

In any case, I got the 80% solution here: 723d10d

But as it turns out there is no built-in AspectFill for an NSImage... So just need to add that and this should be good again.

@jfversluis jfversluis added e/2 🕑 2 p/macOS and removed s/unverified New report that has yet to be verified labels Oct 11, 2019
@jfversluis jfversluis moved this from New to Ready For Work in Triage Oct 11, 2019
@DennisWelu
Copy link
Author

Super work. I believe it did work previously - I have a Mac app that uses AspectFill and the behavior changed after updating. From what I've seen in a couple places the layer has to be captured for the NSImage at the right time then the gravity of the layer can be made to fulfill the AspectFill behavior: https://stackoverflow.com/questions/23002653/nsimageview-image-aspect-fill/38752428#38752428

@jfversluis
Copy link
Member

Thanks @DennisWelu, I've seen that. The weird thing about that code is: the Layer.Content wants a CGImage not an NSImage

@DennisWelu
Copy link
Author

I'm just lofting out observations from casual review of the changes, but I do see it grabbing the CGImage from the NSImage here ce148bf#diff-d1473c83345d2991d27d0b7cb03cf161L112 to set the layer Contents. With the updated structure of the renderer I'm not sure how to best make it adjust between the different aspect values. Would have to study that further.

@jfversluis
Copy link
Member

Wow it must've been Friday afternoon... It simply has a CGImage property 😅.

Implemented it like this and now they all work again. Thanks for the second pair of eyes! I'll open the PR right after this.

@DennisWelu
Copy link
Author

Awesome! Thanks @jfversluis!

@samhouts samhouts added the in-progress This issue has an associated pull request that may resolve it! label Oct 14, 2019
@samhouts samhouts added this to In Progress in v4.3.0 Oct 14, 2019
@samhouts samhouts removed this from Ready For Work in Triage Oct 14, 2019
@samhouts samhouts added this to In Progress in v4.4.0 Oct 18, 2019
@samhouts samhouts removed this from In Progress in v4.3.0 Oct 18, 2019
@samhouts samhouts added this to In Progress in v4.5.0 Nov 2, 2019
@samhouts samhouts removed this from In Progress in v4.4.0 Nov 2, 2019
@DennisWelu
Copy link
Author

I am sad that 2 of 3 aspect settings for Image will continue to not work until vNext+1.
:-(

@jfversluis
Copy link
Member

+1 @DennisWelu sorry about that :( You're more than welcome to take my PR and try to fix that last piece

@DennisWelu
Copy link
Author

@jfversluis LOL, appreciate the sympathy! I did make one comment on the PR as I started to look through the changes a bit. Haven't gotten to the point of ACTUALLY trying to make it work. If I can peel off a couple hours from other projects I might, but can't promise!

@Suplanus
Copy link
Contributor

I have a crazy workaround for scale to full size of the parent control (like window).
Call this on Resizing the parent or setting the image to new size.
You also have to set it to a min value. This could not be null because of #8981

private void ScaleImageToWindowSize(bool visible)
{
    // Have to scale down and then up that macOs is recognize the change 🤦‍♂️
    if (Device.Idiom == TargetIdiom.Desktop)
    {
        this.Dispatcher.BeginInvokeOnMainThread(() =>
        {
            if (visible)
            {
                // Calc needed fullscreen size                    
                double windowMax = Math.Max(this.Width, this.Height);
                double imageMax = Math.Max(ImageBlurred.Width, ImageBlurred.Height);
                double scale = windowMax / imageMax;
                if (scale < 1 || double.IsInfinity(scale))
                {
                    ImageBlurred.Scale = 1;
                }
                else
                {
                    ImageBlurred.Scale = scale + 0.2; // Little offset because there is a black border
                }
            }
            else
            {
                // Have to set this to get original size
                ImageBlurred.HorizontalOptions = LayoutOptions.Center;
                ImageBlurred.VerticalOptions = LayoutOptions.Center;
                ImageBlurred.Scale = 0.0001;
            }
        });
    }
}

@samhouts samhouts added this to In Progress in v4.6.0 Feb 27, 2020
@samhouts samhouts added this to In Progress in vCurrent (4.8.0) Feb 27, 2020
@samhouts samhouts removed this from In Progress in v4.6.0 Mar 2, 2020
@samhouts samhouts removed this from In Progress in v4.5.0 Mar 4, 2020
@samhouts samhouts removed the in-progress This issue has an associated pull request that may resolve it! label Apr 15, 2020
@samhouts samhouts added inactive Issue is older than 6 months and needs to be retested help wanted We welcome community contributions to any issue, but these might be a good place to start! up-for-grabs We welcome community contributions to any issue, but these might be a good place to start! and removed inactive Issue is older than 6 months and needs to be retested help wanted We welcome community contributions to any issue, but these might be a good place to start! labels Jul 31, 2020
@samhouts samhouts added this to the 5.0.0 milestone Aug 13, 2020
@samhouts samhouts removed this from In Progress in vCurrent (4.8.0) Aug 13, 2020
@samhouts samhouts added this to New in Triage Aug 13, 2020
@samhouts samhouts added this to To do in vNext+1 (5.0.0) Aug 14, 2020
@rmarinho rmarinho moved this from New to Ready For Work in Triage Aug 14, 2020
@samhouts samhouts added this to To do in Other Ready For Work Aug 20, 2020
@samhouts samhouts removed this from Ready For Work in Triage Aug 20, 2020
@samhouts samhouts added inactive Issue is older than 6 months and needs to be retested help wanted We welcome community contributions to any issue, but these might be a good place to start! and removed inactive Issue is older than 6 months and needs to be retested labels Sep 18, 2020
@samhouts samhouts removed this from the 5.0.0 milestone Nov 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e/2 🕑 2 help wanted We welcome community contributions to any issue, but these might be a good place to start! p/macOS t/bug 🐛 up-for-grabs We welcome community contributions to any issue, but these might be a good place to start!
Projects
Development

Successfully merging a pull request may close this issue.

4 participants