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

[UWP] Added Windows Platform Specific to avoid set the default image directory #3420

Open
wants to merge 5 commits into
base: master
from

Conversation

10 participants
@jsuarezruiz
Copy link
Collaborator

jsuarezruiz commented Jul 27, 2018

Description of Change

Added Windows Platform Specific to avoid set the default image directory.

Issues Resolved

API Changes

Added:

  • Windows Application Platform Specific in Xamarin.Forms Core project.

Changed:

  • Changes in UWP ImageRenderer.

Platforms Affected

  • Core
  • UWP

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
{
var source = Element.Source;

if (source is FileImageSource)

This comment has been minimized.

@jamesmontemagno

jamesmontemagno Jul 27, 2018

Contributor

Can we use some C# features here: if (source is FileImageSource fileSource && fileSource != null)

This comment has been minimized.

@jsuarezruiz

jsuarezruiz Jul 27, 2018

Author Collaborator

Sure, changed.

@samhouts samhouts added this to In Review in vCurrent (3.6.0) Jul 27, 2018

@samhouts
Copy link
Member

samhouts left a comment

Are we expecting this to be in addition to or instead of #3263?


namespace Xamarin.Forms.Controls.Issues
{
[Preserve(AllMembers = true)]

This comment has been minimized.

@samhouts

samhouts Jul 27, 2018

Member

insert #if APP here

This comment has been minimized.

@jsuarezruiz

jsuarezruiz Jul 27, 2018

Author Collaborator

Added.


}
}
}

This comment has been minimized.

@samhouts

samhouts Jul 27, 2018

Member

insert #endif here

This comment has been minimized.

@jsuarezruiz

jsuarezruiz Jul 27, 2018

Author Collaborator

Added.


if (string.IsNullOrEmpty(directory) || !Path.GetFullPath(directory).Equals(Path.GetFullPath(imageSearchDirectory)))
{
filePath = string.Format("{0}{1}{2}", imageSearchDirectory, Path.DirectorySeparatorChar, filePath);

This comment has been minimized.

@samhouts

samhouts Jul 27, 2018

Member

Why not Path.Combine?

This comment has been minimized.

@jsuarezruiz

jsuarezruiz Jul 27, 2018

Author Collaborator

There is no reason and in fact, it is better. Changed. Thanks!.

@jsuarezruiz

This comment has been minimized.

Copy link
Collaborator Author

jsuarezruiz commented Jul 27, 2018

Ops, I did not see this PR and it's interesting. It's a good question. Maybe are compatible and both can be added. @davidortinau, what do you think?

@davidortinau

This comment has been minimized.

Copy link
Contributor

davidortinau commented Jul 27, 2018

Do we need to go with a platform specific? Could this be a config setting done at or near Xamarin.Forms.Init()?

@jassmith @samhouts

@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

jamesmontemagno commented Jul 27, 2018

That was my other idea too @davidortinau looking at it. I only ever need to set it once really IMHO.

@samhouts

This comment has been minimized.

Copy link
Member

samhouts commented Jul 27, 2018

For @jamesmontemagno's request, a config setting is perfect. We can probably support @MartinZikmund's request for subfolders (#2235) in a different way altogether, closer to @paymicro's solution in #3263 but with a core property instead of a platform specific.

@VladislavAntonyuk

This comment has been minimized.

Copy link

VladislavAntonyuk commented Jul 29, 2018

Will it solve the same problem for wpf?

@dotMorten

This comment has been minimized.

Copy link
Contributor

dotMorten commented Sep 21, 2018

Why doesn't XF have something like WPF's pack URIs that works cross-plat and looks up the right way. Similarly we need a xplat build action that does the right thing per platform

@samhouts samhouts moved this from In Review to In Progress in vCurrent (3.6.0) Sep 21, 2018

@SamuelDebruyn

This comment has been minimized.

Copy link

SamuelDebruyn commented Oct 18, 2018

This line in the ButtonRenderer also has to be modified: https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.UAP/ButtonRenderer.cs#L169

Otherwise the ButtonRenderer will still look in the root for images set as Image on a Button.

Shouldn't we just change https://github.com/xamarin/Xamarin.Forms/blob/bd31e1e9fc8b2f9ad94cc99e0c7ab058174821f3/Xamarin.Forms.Platform.UAP/FileImageSourceHandler.cs so that it picks the image from the right folder?

@samhouts samhouts added the p/UWP label Nov 2, 2018

@PureWeen PureWeen requested review from kingces95 and PureWeen Nov 2, 2018

@samhouts samhouts added the e/5 🕔 label Nov 27, 2018


var directory = Path.GetDirectoryName(filePath);

if (string.IsNullOrEmpty(directory) || !Path.GetFullPath(directory).Equals(Path.GetFullPath(imageSearchDirectory)))

This comment has been minimized.

@kingces95

kingces95 Nov 27, 2018

Member

Why not just drop this if and always combine? Certainly would be easier to document:

If the plat specific is set, then we prepend the search path to all asset paths.

@kingces95
Copy link
Member

kingces95 left a comment

Just a little comment. If there's a reason for the if that I'm missing I'm happy to approve.

@samhouts samhouts removed the request for review from PureWeen Nov 29, 2018

@samhouts samhouts added this to In Progress in vNext (4.0.0) Feb 2, 2019

@samhouts samhouts removed this from In Progress in vCurrent (3.6.0) Feb 2, 2019

@MartinZikmund

This comment has been minimized.

Copy link
Contributor

MartinZikmund commented Feb 22, 2019

Is this still considered? It would be great if it were part of Xamarin.Forms soon :-) .

@samhouts samhouts added this to In Progress in vNext+1 (master) Mar 2, 2019

@samhouts samhouts removed this from In Progress in vNext (4.0.0) Mar 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.