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

GH-125 Take Screenshot#744

Closed
Mrnikbobjeff wants to merge 59 commits intoxamarin:mainfrom
Mrnikbobjeff:feature/Screenshot
Closed

GH-125 Take Screenshot#744
Mrnikbobjeff wants to merge 59 commits intoxamarin:mainfrom
Mrnikbobjeff:feature/Screenshot

Conversation

@Mrnikbobjeff
Copy link
Copy Markdown
Contributor

@Mrnikbobjeff Mrnikbobjeff commented Mar 19, 2019 •
edited
Loading

Description of Change

API Changes

Added:

    {
        public static bool CanCapture => PlatformCanCapture;

        public static Task<MediaFile> CaptureAsync()
            => PlatformCaptureAsync();
    }

And the same media file as in #178

Docs update can come once we decide on the exact mediafile structure..

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)

@ghost

This comment has been minimized.

Sign in to view
@Mrnikbobjeff
Copy link
Copy Markdown
Contributor Author

Mrnikbobjeff commented Mar 19, 2019

@syed-pelican @mattleibow ping, new pr same code

@Mrnikbobjeff Mrnikbobjeff changed the title [WIP] GH-125 Take Screenshot GH-125 Take Screenshot Mar 19, 2019
@syed-pelican
Copy link
Copy Markdown

syed-pelican commented Mar 20, 2019

and the merge failed.....

Finished Samples, added implementations for missing platforms. Added …
24024e7 
…same media file as in other struct

# Conflicts:
#	Xamarin.Essentials/Platform/Platform.android.cs
@ghost

This comment has been minimized.

Sign in to view
mattleibow
mattleibow reviewed Mar 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@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.

Thanks for this PR. It is much better than the last one and looks very good.

We did just add a new base FileBase type, so see if you can use that.

Xamarin.Essentials/Screenshot/Screenshot.ios.cs Outdated
{
internal static bool PlatformCanCapture => UIScreen.MainScreen != null;

static string GetTempFileName()
Copy link
Copy Markdown
Member

@mattleibow mattleibow Mar 21, 2019

Choose a reason for hiding this comment

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

Can we make use of the members on the FileSystem type? If not, it might be better to add something there for other APIs to use.

Copy link
Copy Markdown
Contributor Author

@Mrnikbobjeff Mrnikbobjeff Mar 21, 2019

Choose a reason for hiding this comment

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

No, the current FileSystem type does not provide the necessary functionality. Adding it does sound like a good idea. See the other post

@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
newky2k
newky2k suggested changes May 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@newky2k newky2k left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Now that the PR is upto date with master there are some build errors for the android implementation

Also need to include the documentation changes to the API.

Thanks in Advance

@newky2k newky2k added bug Something isn't working DO-NOT-MERGE Don't merge it.... don't do it! needs-specifications Accepted feature/enhancement and needs specification. labels May 28, 2019
@jamesmontemagno jamesmontemagno added the community Community contributions label Aug 1, 2019
@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
@Mrnikbobjeff
Copy link
Copy Markdown
Contributor Author

Mrnikbobjeff commented Nov 20, 2019

@newky2k Would you mind taking a look at it again? I added docs and removed the deprecated method. I really would like to not have to merge master changes on this many branches :)

@ghost

This comment has been minimized.

Sign in to view
mattleibow
mattleibow reviewed Nov 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@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.

Very nice! I have a few comments, mostly things that popped to mind as I was reading. I do this this API, neat and clean.

Mrnikbobjeff
Mrnikbobjeff commented Nov 22, 2019
View reviewed changes
@ghost

This comment has been minimized.

Sign in to view
Schilli, Niklas added 2 commits November 23, 2019 18:46
Moved all files to cache location, changed the home icon for screensh…
574efb0 
…ot feature, changed method signature to be compatible with the picker as suggested. Also renamed mediafile to screenshotfile
Updated docs
90a0c20
@ghost

This comment has been minimized.

Sign in to view
@Mrnikbobjeff
Copy link
Copy Markdown
Contributor Author

Mrnikbobjeff commented Nov 23, 2019

@mattleibow I moved all files to the cache location, changed the icon and renamed the AsStream method including the signature change. Should be ready to go.

@ghost

This comment has been minimized.

Sign in to view
mattleibow
mattleibow reviewed Nov 24, 2019
View reviewed changes
Xamarin.Essentials/Screenshot/Screenshot.android.cs
{
static bool PlatformCanCapture => !Platform.WindowManager.DefaultDisplay.Flags.HasFlag(DisplayFlags.Secure);

static string GetTempFileName() => Path.Combine(FileSystem.CacheDirectory, Path.GetTempFileName());
Copy link
Copy Markdown
Member

@mattleibow mattleibow Nov 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@jamesmontemagno @Redth I am still not really happy with this... This folder is publicly available to the user, so should we be putting things in here? If the user does not expect this, what happens? Should we have a special folder in the cache where we work? Should we truly use some OS temp system?

@Mrnikbobjeff previously had it in the external storage folder, and I sort of felt it is better in the cache folder. I think it is better now, but maybe we need to decide on what we want to do with files we create.

Copy link
Copy Markdown
Member

@mattleibow mattleibow Nov 24, 2019

Choose a reason for hiding this comment

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

@Mrnikbobjeff I am thinking this is probably better to just be in the PlatformCaptureAsync method as it is a single line.

Copy link
Copy Markdown
Contributor Author

@Mrnikbobjeff Mrnikbobjeff Nov 28, 2019

Choose a reason for hiding this comment

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

An interesting question would be to see how to save files in the application cache folder, as the user can clear than themself.

Copy link
Copy Markdown
Contributor Author

@Mrnikbobjeff Mrnikbobjeff Nov 28, 2019

Choose a reason for hiding this comment

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

I would like to save this in the application cache folder, as the user can clear that manually

Xamarin.Essentials/Screenshot/Screenshot.ios.tvos.cs

static string GetTempFileName()
{
var tempFileName = Path.ChangeExtension(Path.Combine(FileSystem.CacheDirectory, Path.GetTempFileName()), ".png");
Copy link
Copy Markdown
Member

@mattleibow mattleibow Nov 24, 2019

Choose a reason for hiding this comment

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

To be consistent, we should probably use Path.Combine(FileSystem.CacheDirectory, Path.GetTempFileName()); like Android. Also, as it is a single line, just put it in the PlatformCaptureAsync method.

Xamarin.Essentials/Screenshot/Screenshot.uwp.cs
static async Task<ScreenshotFile> PlatformCaptureAsync()
{
var element = Window.Current.Content as FrameworkElement;
var fileOnDisk = await ApplicationData.Current.LocalCacheFolder.CreateFileAsync(Path.ChangeExtension(Path.GetRandomFileName(), ".png"));
Copy link
Copy Markdown
Member

@mattleibow mattleibow Nov 24, 2019

Choose a reason for hiding this comment

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

To be consistent, we should probably use Path.Combine(FileSystem.CacheDirectory, Path.GetTempFileName()); like Android.

Copy link
Copy Markdown
Contributor Author

@Mrnikbobjeff Mrnikbobjeff Nov 28, 2019

Choose a reason for hiding this comment

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

I actually prefer the uwp implementation over all others, as this allows the user to remove the cached data by cleaning app data.

Copy link
Copy Markdown
Contributor Author

@Mrnikbobjeff Mrnikbobjeff Nov 28, 2019

Choose a reason for hiding this comment

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

I like this more as the user can clear application cache directly

@mattleibow
Copy link
Copy Markdown
Member

mattleibow commented Nov 24, 2019

I think this is really close, that location where we save temp files is still bugging me.

@Mrnikbobjeff
Copy link
Copy Markdown
Contributor Author

Mrnikbobjeff commented Nov 29, 2019

Should we use Context.CacheDir on Android? https://docs.microsoft.com/en-us/dotnet/api/android.content.context.cachedir?view=xamarin-android-sdk-9

@Mrnikbobjeff
Copy link
Copy Markdown
Contributor Author

Mrnikbobjeff commented May 5, 2020

Are there any plans to finally include this branch into the upcoming release? There has been virtually no developement on this branch except deciding where to store the file in the last 8 months. We are already rolling our own EssentialsEx package including this and other branches which remained virtually finished for quite a while. @jamesmontemagno @mattleibow

@mattleibow mattleibow changed the base branch from master to main June 9, 2020 13:49
@mattleibow mattleibow mentioned this pull request Aug 20, 2020
Merged
5 tasks
mattleibow added a commit that referenced this pull request Aug 25, 2020
@mattleibow
Merge pull request #1356 from xamarin/dev/screenshots
5e7d533 
Screenshots #744 -> #613
@Redth
Copy link
Copy Markdown
Member

Redth commented Sep 1, 2020

Merged in #1356

@Redth Redth closed this Sep 1, 2020
@Mrnikbobjeff Mrnikbobjeff deleted the feature/Screenshot branch September 1, 2020 18:38
@zleao zleao mentioned this pull request Oct 8, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@mattleibow mattleibow mattleibow left review comments

@newky2k newky2k newky2k requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working community Community contributions DO-NOT-MERGE Don't merge it.... don't do it! needs-specifications Accepted feature/enhancement and needs specification.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.