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

Add Share Source Position #1002

Merged
merged 10 commits into from Dec 9, 2019
Merged

Add Share Source Position #1002

merged 10 commits into from Dec 9, 2019

Conversation

Redth
Copy link
Member

@Redth Redth commented Nov 25, 2019

Description of Change

Adds the ability to specify a Source Rectangle for Share*Requests. This is really only used for now on iOS where on some iOS 13 versions it seems the sourceRect needs to be set in order for the dialog to display.

This should address #980 #979 #944 by allowing users to set their own source position.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Updated documentation (see walkthrough)

@VSC-Service-Account

This comment has been minimized.

@VSC-Service-Account

This comment has been minimized.

@Redth Redth marked this pull request as ready for review November 25, 2019 21:47
@VSC-Service-Account

This comment has been minimized.

mattleibow
mattleibow previously approved these changes Nov 25, 2019
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Other than the one comment on the name, LGTM.

Xamarin.Essentials/Share/Share.shared.cs Outdated Show resolved Hide resolved
@VSC-Service-Account

This comment has been minimized.

@Redth
Copy link
Member Author

Redth commented Nov 25, 2019

We were going to add sensible presets like TopLeft, TopRight, etc. however calculating this on each platform is a bit out of scope of this work for now. It should have some more thought and discussion around what that means for each platform and what those coordinates look like for things like the status bars, etc. For now I think it's best we enable this functionality to unblock iOS iPad apps.

@VSC-Service-Account

This comment has been minimized.

public string Title { get; set; }

#if !NETSTANDARD1_0
public Rectangle PresentationSourceBounds { get; set; } = Rectangle.Empty;
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL for netstandard1.x

@VSC-Service-Account

This comment has been minimized.

@VSC-Service-Account

This comment has been minimized.

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Looks good. Name is better. The community has until @Redth gets back from lunch to decide. ;)

@VSC-Service-Account

This comment has been minimized.

@bobwhitten
Copy link

Thanks @Redth. I've tested this and it works. But I am concerned that some will be disappointed to find that if they don't set this SourceScreenPosition they will NOT get this to work on the iPad with 13.2. For example, in the current sample code.

Would an additional comment in the documentation clarify?

@Redth
Copy link
Member Author

Redth commented Nov 25, 2019

I suppose we could set a default for the case where it’s a Tablet idiom and iOS. Do you think Rectangle(width /2, height /2, 0, 0) is a sane default for this?

@bobwhitten
Copy link

bobwhitten commented Nov 26, 2019

I think center of screen is as good as any, though someone has asked for top right (#944).

Most important thing is that this sourceRect get set in the iPad case.

@jamesmontemagno
Copy link
Collaborator

LGTM

@Redth Redth merged commit c6dde15 into master Dec 9, 2019
@Redth Redth deleted the ios-share-location branch December 9, 2019 16:51
@hemersonvarela
Copy link

If I'm allowed to ask, when will be release that includes this PR?

@Redth
Copy link
Member Author

Redth commented Dec 11, 2019

We are working on a 1.4.0. I can’t give you a date yet but trying to sort out a bunch of PRs first to issue a release.

@RuddyOne
Copy link

RuddyOne commented Dec 19, 2019

@Redth In the future would it be possible to release hot fixes without including them in a bigger release? #980 was a super simple fix and by the looks of it would have helped a lot of people with a quick release (myself included).

As we do not have a release date for 1.4.0 it is forcing some of us to fork this project to make the changes ourselves (as I will have to do now). Would be great if in the future we could treat these types of scenarios for what they really are... a hot fix with a quick release.

@Redth
Copy link
Member Author

Redth commented Dec 19, 2019

Yes, we can try and do smaller fix releases in the future. I would also remind everyone that you can go grab a .nupkg to use yourself: https://dev.azure.com/xamarin/public/_build?definitionId=7&_a=summary

We will look at creating some sort of nightly package feed for master builds which would help even more for these cases.

@mauro3005
Copy link

Is this fix currently in the 1.4.0 pre-release? I installed the 1.4.0 pre-release nuget and Share sheet is still not showing on iPad iOS 13.3. I just see a little white dot in the top left corner when I tap share from the nav bar and if I tap again white dot goes away.

@jamesmontemagno
Copy link
Collaborator

jamesmontemagno commented Jan 9, 2020

You can set the

PresentationSourceBounds = DeviceInfo.Platform== DevicePlatform.iOS && DeviceInfo.Idiom == DeviceIdiom.Tablet
                            ? new System.Drawing.Rectangle(0, 20, 0, 0)
                            : System.Drawing.Rectangle.Empty

@mauro3005
Copy link

You can set the

PresentationSourceBounds = DeviceInfo.Platform== DevicePlatform.iOS && DeviceInfo.Idiom == DeviceIdiom.Tablet
                            ? new System.Drawing.Rectangle(0, 20, 0, 0)
                            : System.Drawing.Rectangle.Empty

Thank you James! This does indeed work with that PresentationSourceBounds Property set on 1.4.0 pre-release. We will be on the lookout for this on latest stable :)

@MalukuSeito
Copy link

I tried this, however, now I can't run it on Android, I get the following exception:
System.TypeLoadException: Could not set up parent class, due to: Could not load type of field 'Xamarin.Essentials.ShareRequestBase:<PresentationSourceBounds>k__BackingField' (1) due to: Could not resolve type with token 0100008d from typeref (expected class 'System.Drawing.Rectangle' in assembly 'System.Drawing.Common, Version=4.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51') assembly:System.Drawing.Common, Version=4.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 type:System.Drawing.Rectangle member:(null)

I updated all projects to the same version and added System.Drawing.Common to the imported packages, but it's no help.
Any idea what might be missing? It worked fine on android before pre-2.

ScottBTR pushed a commit to builttoroam/Essentials that referenced this pull request Jan 22, 2020
* Add SourceScreenPosition to Share*

* Rename base type to be more specific

* Add docs for Share changes

* Add SourceScreenPosition to sample

* Rename source property

* Fixed docs from rename

* Fix docs

* Another docs fix

* Make abstract
@StevoKeano
Copy link

StevoKeano commented Feb 6, 2021

No popup.... Ran in 1/3 screen and full screen on the Model: iPad6,11 iPad Generation 5 - A1822. Cannot believe I'm debugging over wifi from windows; but, this single call is broken.. Got here in 12 hours from Dev Account creation to almost there... Are there any solutions? XAMARIN.FORMS 5.0.0.1931, ESSENTIALS 1.6.1 iOS 14.4
1

        private static async Task ShareFile(string nNumber, string fltLogCSVOutputPath)
        {
            await Share.RequestAsync(new ShareFileRequest
            {
                Title = nNumber + " ShareFile",
                File = new ShareFile(fltLogCSVOutputPath),
                // PresentationSourceBounds = new System.Drawing.Rectangle(0, 0, 600, 300)
                PresentationSourceBounds = DeviceInfo.Platform == DevicePlatform.iOS && DeviceInfo.Idiom == DeviceIdiom.Tablet
                            ? new System.Drawing.Rectangle(0, 20, 0, 0)
                            : System.Drawing.Rectangle.Empty
            }); ;
        }

@mattleibow
Copy link
Contributor

@StevoKeano is it possible to open a new issue so we can track this properly?
Also, what happens when you run the share sample in the sample app in this repo?

Could you maybe attach a sample? I just did a test and it shows up for me. Maybe you are not doing it on the UI thread?

@StevoKeano
Copy link

StevoKeano commented Feb 6, 2021

Using the UI Thread I wasn't. Thanks @mattleibow ! Funny it worked on Android...

@mattleibow
Copy link
Contributor

iOS is stricter. Anything that touches the UI at all must be on the UI thread.

@StevoKeano
Copy link

Used TaskCompletionSource to get the (a)wait effect...
` private Task ShareFile(string nNumber, string fltLogCSVOutputPath)

    {
        var tcs = new TaskCompletionSource<bool>();
        Device.BeginInvokeOnMainThread(() =>
        {
          Share.RequestAsync(new ShareFileRequest
                {
                    Title = nNumber + " ShareFile",
                    File = new ShareFile(fltLogCSVOutputPath),
                    // PresentationSourceBounds = new System.Drawing.Rectangle(0, 0, 600, 300)
                    PresentationSourceBounds = DeviceInfo.Platform == DevicePlatform.iOS && DeviceInfo.Idiom == DeviceIdiom.Tablet
                        ? new System.Drawing.Rectangle(0, 20, 0, 0)
                        : System.Drawing.Rectangle.Empty
                });
        });
        return tcs.Task;
    }`

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

Successfully merging this pull request may close these issues.

None yet

10 participants