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

Remote Source Tile View Implementation #518

Merged
merged 18 commits into from
Oct 30, 2017
Merged

Remote Source Tile View Implementation #518

merged 18 commits into from
Oct 30, 2017

Conversation

gep13
Copy link
Member

@gep13 gep13 commented Oct 27, 2017

@RichiCoder1 @ferventcoder @punker76 @mwallner @pascalberger This is VERY much a Work In Progress, but I just wanted to put this out there for discussion.

What are your immediate thoughts on this?

User can switch between List and Tile view using buttons here:

image

Existing list view stays exactly the same as it is, but when switching to Tile, you see "something" like the following:

image

Interesting side effect of the InternetImage control that we are using, that when you first hit the new tile view, you see something like this:

image

So thoughts? Good idea? Bad idea? What needs to be different?

@ferventcoder
Copy link
Member

Great start! I think we should have something for the placeholder images. Definitely like what I am seeing.

@punker76
Copy link
Member

punker76 commented Oct 27, 2017

@gep13 first thoughts

  • caching images, cause switching to list and back loads it again
  • ListView and ListViewItem style doesn't use MahApps one
  • ToggleSwitch for the 2 views instead radio buttons
  • show also the version

@gep13
Copy link
Member Author

gep13 commented Oct 27, 2017

@punker76 yes, image caching was something that had occurred to me as well. Would need to extend the current control to look in the local cache before reaching out to the interwebs.

@RichiCoder1
Copy link
Contributor

  • caching images, cause switching to list and back loads it again

That's weird. If you're using the same control as on PackageView, it is caching. Maybe the the delay is reading from disk?

  • ToggleSwitch for the 2 views instead radio buttons

Agreed.

  • show also the version

As well as install/outdated status, preferably.

@pascalberger
Copy link
Contributor

@gep13 Sounds like a good idea to me. Also would like to see version and status (installed, outdated, pinned) in the view (maybe as overlays)

@gep13
Copy link
Member Author

gep13 commented Oct 28, 2017

@RichiCoder1 yip, it is the same control as on the package details view. Perhaps the issue is the number of requests at the same time.

@punker76
Copy link
Member

@RichiCoder1 @gep13 @pascalberger @ferventcoder Something for the eye...

new mockup 1

@gep13
Copy link
Member Author

gep13 commented Oct 28, 2017

@punker76 to me, this looks like exactly what we want 😄

I think the word New might want to be another word, but not immediately sure on what it should be. Part of me wants to put Upgrade available but that is obviously too much.

@punker76
Copy link
Member

@gep13 out-of-date

@punker76
Copy link
Member

@gep13

new mockup 2

@gep13
Copy link
Member Author

gep13 commented Oct 28, 2017

Or perhaps the term outdated, which is what Chocolatey itself uses should be in there.

Either way, it is a small point, everything else looks 👍

@gep13
Copy link
Member Author

gep13 commented Oct 28, 2017

@RichiCoder1 there is something up with the caching implementation. Sometimes the Metadata isn't set, which means it goes back out to the internet to fetch the image again. Sometimes it works, sometimes it doesn't 😢

  The InternetImage try to cache the images for 1 day and do this by adding an expired date as meta data.
  Setting this with the SetMetadata method for the file storage doesn't work. Requesting of this meta data returns a undefined DateTime.
  But setting this directly to the meta data of the uploaded image stream works perfectly.
@punker76
Copy link
Member

@gep13 @RichiCoder1 Caching images for InternetImage should be fixed now.

The InternetImage try to cache the images for 1 day and do this by adding an expired date as meta data. Setting this with the SetMetadata method for the file storage doesn't work. Requesting of this meta data returns a undefined DateTime.

But setting this directly to the meta data of the uploaded image stream works perfectly.

  use StretchDirection DownOnly
  use PackIconEntypoKind.CircleWithCross for error
  use PackIconEntypoKind.Block for empty urls
  extract PackageListTemplate and PackageTileTemplate from ListViewItem
  use PackagesContextMenu for PackageTileTemplate too
  TextTrimming for title
@punker76
Copy link
Member

@gep13 Intermediate result...

2017-10-28_22h22_52

Copy link
Contributor

@RichiCoder1 RichiCoder1 left a comment

Choose a reason for hiding this comment

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

Really liking where this is going! Maybe something like this?:
image

For context, my inspiration was basically Google Play & Windows Store.

@@ -36,7 +35,8 @@ public partial class InternetImage
"IconUrl", typeof(string), typeof(InternetImage), new PropertyMetadata(default(string)));

private static readonly ILogger Logger = Log.ForContext<InternetImage>();
private static readonly Lazy<BitmapSource> ErrorIcon = new Lazy<BitmapSource>(GetErrorImage);
private static readonly Lazy<ImageSource> ErrorIcon = new Lazy<ImageSource>(() => GetPackIconEntypoImage(PackIconEntypoKind.CircleWithCross, Brushes.OrangeRed));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mwallner
Copy link
Member

to throw my fife cents in: I love what's going on, but I'd prefer a solution close to what Richard suggested - having more concrete borders around elements from the first place, without selecting anything .. (I think it'd be a cleaner interface)

@punker76
Copy link
Member

@mwallner I've thought of that, too...

new mockup 3

@punker76
Copy link
Member

punker76 commented Oct 29, 2017

@mwallner @gep13

image

or only 1 BorderThickness

image

@gep13
Copy link
Member Author

gep13 commented Oct 29, 2017

@punker76 oooo, this is really starting to shape up, I like it!

One point... I think the name of the application should appear above the version number.

Also, while I think about it, while I don't dislike the current no icon symbol, I had thought about using a "washed out" version of the Chocolatey Icon, when there isn't a Icon Url defined.

What are your thoughts on that?

@punker76
Copy link
Member

@gep13 what do you mean with "washed out"?

Name above version...

image

  add own ListViewItemTileStyle which looks now more like a tile
  show IsPinned status
  name above version
@punker76
Copy link
Member

punker76 commented Oct 29, 2017

@gep13 @RichiCoder1 @ferventcoder maybe I don't know enough about it, but it seems this could work...

in RemoteSourceViewModel

                    var installed = await _chocolateyPackageService.GetInstalledPackages();
                    var outdated = await _chocolateyPackageService.GetOutdatedPackages();

                    PageCount = (int)(((double)result.TotalCount / (double)PageSize) + 0.5);
                    Packages.Clear();
                    result.Packages.ToList().ForEach(p =>
                    {
                        if (installed.Any(package => package.Id == p.Id))
                        {
                            p.IsInstalled = true;
                        }
                        if (outdated.Any(package => package.Item1 == p.Id))
                        {
                            p.IsLatestVersion = false;
                        }

                        Packages.Add(Mapper.Map<IPackageViewModel>(p));
                    });

2017-10-29_12h11_37

@punker76
Copy link
Member

@gep13 @RichiCoder1 @ferventcoder @pascalberger I think I'm done for now, last one is the RadioButton style for the ListViewMode...

2017-10-29_14h47_48

2017-10-29_14h47_54

@ferventcoder
Copy link
Member

This looks amazing!! The only thing I might change - a watermarked default image when no image is available - that greyed out choco icon (maybe previously mentioned).

@gep13
Copy link
Member Author

gep13 commented Oct 29, 2017

@ferventcoder I am assuming in this case that you are ok with the official Chocolatey Logo being modified? Or should this be an icon that makes it's way into the official Media Kit for Chocolatey?

@ferventcoder
Copy link
Member

This is an official Chocolatey client so 👍

@punker76
Copy link
Member

@gep13 @ferventcoder

2017-10-30_10h01_04

@punker76
Copy link
Member

@gep13 @ferventcoder also on all other places

2017-10-30_10h06_48

  Faster image loading:
    we don't need to delete the file, cause a upload does: Send file or stream to database. Can be used with file or Stream. If file already exists, file content is overwritten.
    do not lock over the entire operation
@gep13
Copy link
Member Author

gep13 commented Oct 30, 2017

@punker76 so, where are we with this? Is there anything left that you are looking to add? Or is this ready for review?

Should/could we add the same tile view to the Local Packages Tab as well?

Should/could we add an application level setting which specifies the default view to use within the application?

@punker76
Copy link
Member

so, where are we with this? Is there anything left that you are looking to add? Or is this ready for review?

I'm done

Should/could we add the same tile view to the Local Packages Tab as well?

That would be consistent. But we should do this maybe in a new PR.

Should/could we add an application level setting which specifies the default view to use within the application?

👍

@gep13
Copy link
Member Author

gep13 commented Oct 30, 2017

@punker76 agree on new PR's for other pieces of work. Let's get this part reviewed and pulled in, and we can discuss further.

I will try to take it for a proper spin tonight.

@pascalberger
Copy link
Contributor

pascalberger commented Oct 30, 2017

I tried to install version from this build over my previous installation but after switching to tile view on the feed I received this error:

image

Doesn't seem to be able to reproduce it though

  so use `string.Equals(package.Id, p.Id, StringComparison.OrdinalIgnoreCase)`
  instead `package.Id == p.Id`
@punker76
Copy link
Member

@pascalberger Which prev version? The 0.15 or one prev of these PR? And can you try it again with the latest from this PR? Thx.

@pascalberger
Copy link
Contributor

@punker76 It was a 0.16 prev version installed over which I installed the version from this PR

@pascalberger
Copy link
Contributor

Couldn't reproduce it with the latest build from this PR (but couldn't also reproduce it before either :) )

@RichiCoder1
Copy link
Contributor

Hmm. Looks like I didn't set a switch on Image Magick's svg conversion to use transparent backgrounds. Will need to enable that....

@gep13 gep13 changed the title [WIP] Tile View Implementation Tile View Implementation Oct 30, 2017
@gep13
Copy link
Member Author

gep13 commented Oct 30, 2017

@punker76 just had a play with this, and from what I can see, we can :shipit: and get some more feedback.

@gep13 gep13 merged commit 42c5351 into develop Oct 30, 2017
@gep13 gep13 deleted the feature/tile-view branch October 30, 2017 22:04
@punker76
Copy link
Member

@gep13 👏 so let me create the next one for the local view 😉

choco_local_tileview

2017-10-30_22h52_35

@gep13 gep13 changed the title Tile View Implementation Remote Source Tile View Implementation Oct 30, 2017
@gep13
Copy link
Member Author

gep13 commented Oct 30, 2017

Relates to #517

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.

6 participants