Desktop photo search #174

domesticmouse · 2019-12-09T02:51:55Z

Adding a desktop sample app.

domesticmouse · 2019-12-09T03:10:44Z

FYI @csells

…top_photo_search

redbrogdon

Some drive-by comments. I'll try to get back to this ASAP.

experimental/desktop_photo_search/README.md

experimental/desktop_photo_search/analysis_options.yaml

experimental/desktop_photo_search/lib/main.dart

redbrogdon · 2019-12-09T22:44:23Z

experimental/desktop_photo_search/lib/main.dart

+  @override
+  Widget build(BuildContext context) {
+    final photoSearchModel = Provider.of<PhotoSearchModel>(context);
+    menubar.setApplicationMenu([


Is this something that could/should be moved out of the build method? The fact that the widget's being built doesn't necessarily imply it's actively on the screen.

The menu action has an implicit linkage to this window through the callback's construction of a dialog in the window. The only way I can see to make this work is for this code to be run during build, so that it has access to the build context for showDialog. Happy to take guidance if my understanding of this constraint is incorrect.

You're right, the need for a context weirds things up. I was mostly operating on my "build methods should avoid side effects wherever possible" instincts.

This reminds me of a situation I went to Hans for advice on: I wanted a network refresh for some data to be kicked off whenever the app navigated to a widget (DetailsScreen, I think it was). I had the update call initiating in a build method, but IIRC Hans suggested having my own Route object to do it.

At the moment, I don't have any real advice to offer, so stick with what you've got. I'm curious now, though, and I may ask Hans about it next week. 😄

The fun part is that the menu will change depending on which window is selected for input in the user's window stack, which is independent of the repaint cycle.

experimental/desktop_photo_search/lib/main.dart

experimental/desktop_photo_search/lib/src/model/photo_search_model.dart

redbrogdon · 2019-12-09T22:59:35Z

experimental/desktop_photo_search/lib/src/widgets/photo_details.dart

+  final PhotoDetailsPhotoSaveCallback onPhotoSave;
+
+  @override
+  Widget build(BuildContext context) => SingleChildScrollView(


nit: Generally, arrow syntax should be used for simple one-liners. While it works here, it increase rightward drift of the code.

Converted back to block syntax

I can break this out into builder methods and/or subsidiary widgets. Thoughts?

Broken out some of the content into a builder method, and added some animation to make the initial transition from no loaded image to loaded image less janky.

redbrogdon · 2019-12-10T03:46:10Z

experimental/desktop_photo_search/lib/main.dart

+  @override
+  Widget build(BuildContext context) {
+    final photoSearchModel = Provider.of<PhotoSearchModel>(context);
+    menubar.setApplicationMenu([


You're right, the need for a context weirds things up. I was mostly operating on my "build methods should avoid side effects wherever possible" instincts.

This reminds me of a situation I went to Hans for advice on: I wanted a network refresh for some data to be kicked off whenever the app navigated to a widget (DetailsScreen, I think it was). I had the update call initiating in a build method, but IIRC Hans suggested having my own Route object to do it.

At the moment, I don't have any real advice to offer, so stick with what you've got. I'm curious now, though, and I may ask Hans about it next week. 😄

experimental/desktop_photo_search/lib/main.dart

redbrogdon · 2019-12-10T04:05:48Z

experimental/desktop_photo_search/lib/src/unsplash/unsplash.dart

+
+    if (body is Map && body['errors'] is List && body['errors'].isNotEmpty) {
+      final apiError = ApiError.fromJson(response.body);
+      throw UnsplashException(apiError.errors.join(', '));


Consider trapping other issues (json failing to parse on line 64, for instance) and returning this exception class as well.

Added handling for invalid JSON response

redbrogdon · 2019-12-10T04:06:59Z

experimental/desktop_photo_search/lib/src/widgets/data_tree.dart

@@ -0,0 +1,350 @@
+// Copyright 2019 The Flutter team. All rights reserved.


Is this something you wrote in concert with Hans, or is it something he did for a different project that also works here?

This code was lifted and adapted from a gist put together by @HansMuller.

Hey @csells, the original idea for this sample was to showcase both Split and DataTree, but I fear the complexity of the DataTree in this sample is working against our larger goals of making Flutter on desktop approachable. Would you be ok with taking DataTree out and replacing it with something simpler? The upside is that we get a path to introducing shared preferences to store favorited photos.

redbrogdon · 2019-12-10T04:13:31Z

experimental/desktop_photo_search/lib/src/widgets/split.dart

@@ -0,0 +1,194 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.


This has a different author name than the others. Is it pulled from something else?

Yup, it's @DaveShuckerow's code, and lifted and adapted from Flutter DevTool Widget Inspector, if memory serves.

domesticmouse added 2 commits December 9, 2019 13:40

Initial commit

8573cfe

Run through Xcode 11.2.1 to fix up config

4e55616

domesticmouse requested a review from redbrogdon December 9, 2019 02:51

Add flutter aware clean

3e50764

domesticmouse requested a review from legalcodes December 9, 2019 03:11

legalcodes approved these changes Dec 9, 2019

View reviewed changes

Merge branch 'master' of https://github.com/flutter/samples into desk…

447e183

…top_photo_search

redbrogdon reviewed Dec 9, 2019

View reviewed changes

domesticmouse added 2 commits December 10, 2019 10:45

Code review fixups

48fd90f

Refactor photo attrib, and animate image size.

d41de70

domesticmouse mentioned this pull request Dec 10, 2019

[url_launcher] Adds macOS package with example app flutter/plugins#2383

Merged

13 tasks

domesticmouse added 2 commits December 10, 2019 14:01

Pubspec versions update

d72a0ca

url_launcher_macos

92d28c5

redbrogdon reviewed Dec 10, 2019

View reviewed changes

domesticmouse added 4 commits December 10, 2019 15:57

Simplifying naming of tests.

071d118

Taking out shared preferences

ec5b732

Fix test naming

dd8df6b

Handle invalid JSON

c77ce72

redbrogdon approved these changes Dec 10, 2019

View reviewed changes

domesticmouse merged commit f07b4a9 into flutter:master Dec 10, 2019

domesticmouse deleted the desktop_photo_search branch December 10, 2019 20:22

		@@ -0,0 +1,350 @@
		// Copyright 2019 The Flutter team. All rights reserved.

		@@ -0,0 +1,194 @@
		// Copyright 2019 The Chromium Authors. All rights reserved.

Desktop photo search #174

Desktop photo search #174

Uh oh!

Conversation

domesticmouse commented Dec 9, 2019

Uh oh!

domesticmouse commented Dec 9, 2019

Uh oh!

redbrogdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domesticmouse Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

domesticmouse Dec 10, 2019 •

edited

Loading