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

Added easy_image_viewer with page.open_image_viewer([img_paths]) method #943

Closed
wants to merge 5 commits into from

Conversation

Skquark
Copy link

@Skquark Skquark commented Jan 28, 2023

@CLAassistant
Copy link

CLAassistant commented Jan 28, 2023

CLA assistant check
All committers have signed the CLA.

@FeodorFitsner
Copy link
Contributor

Thank you for your PR!

Some notes:

  1. CI is failing: https://ci.appveyor.com/project/flet-dev/flet/builds/46069865/job/7pdkr4ychaavk47a#L194 - it looks like web target of Flet client cannot be built (flutter build web)?
  2. Image(s) can be not just a URL, but a relative URL or a file path: https://github.com/flet-dev/flet/pull/943/files#diff-88b7ad3980c04fb051a65f27fcbf01fa361d01334f3bf45978f4227c6a0dd43bR18 Take a look at how it's handled for Container.image_src:
    } else if (imageSrc != "") {
    var assetSrc =
    getAssetSrc(imageSrc, pageArgs.pageUri!, pageArgs.assetsDir);
    image = DecorationImage(
    image: assetSrc.isFile
    ? getFileImageProvider(assetSrc.path)
    : NetworkImage(assetSrc.path),
    repeat: imageRepeat,
    fit: imageFit,
    opacity: imageOpacity);
    }
  3. It would be a "cleaner" approach to implement that as a separate ImageViewer control with a simple open = True|False property rather than page.open_image_viewer() method.

@Skquark Skquark changed the title Added easy_image_viewer with page.openImageViewer([urls]) method Added easy_image_viewer with page.open_image_viewer([img_paths]) method Jan 29, 2023
@Skquark
Copy link
Author

Skquark commented Jan 29, 2023

Alright, I made those corrections on the way you get the imageProvider from URL or path. A little trickier than I thought having to wrap it in the StoreConnector builder, and some of my mistakes with null safety, but finally builds with no errors and everything looks correct. Hoping there's no more issues and it works correctly now.
If you really think it's cleaner as a control rather than an invoked method, would need to scrap this branch and start over. I didn't think it should be a control widget since there'd be no interactions with it once the image_viewer dialog is opened. This seemed like the most logical approach to me, but it's up to you...

@FeodorFitsner
Copy link
Contributor

I'd really love to see that as a separate control and don't mix into page or reducers. We are thinking at the moment about Flet client extensibility (aka custom build of Flutter client with user controls) and it's going to be made around controls. I understand there is no consistency right now between pushing something into page as a method or implementing a control, but growing Page class makes me scared. Please make it as a control.

@Skquark
Copy link
Author

Skquark commented Jan 31, 2023

Alright, I'm working on converting that to a Control instead.. Not as simple as I thought since there's no direct example to use as a template, and I'm trying to teach myself the ins-and-outs of your coding as I go. Almost have the new branch ready with that, but trying to iron it out, might need your help to smooth the code. The closest analogy to it was the banner or alert_dialog, but they're not really the same either.
Do I have to add the .open = True since there'd never be an open = False and it doesn't need to be in the widget tree, or can we just make it as a direct invoker to open when called? It's simpler to call it like ft.ImageViewer(image_list) intead of page.dialog = ft.ImageViewer(image_list), page.dialog.open = True, page.update to do the same. Trying to keep things uncomplicated.

@Skquark
Copy link
Author

Skquark commented Jan 31, 2023

Finished up the ImageViewer Control and created a new PR #960 for it. I really hope I did everything right and didn't leave out any details to integrate it in. I didn't test it, but everything looks right. Please review and make any modifications needed, did my best to save you the effort. I really appreciate the work put into Flet, and I look forward to using the full screen image viewer all throughout my personal app. Thanks...

@nnko0o
Copy link

nnko0o commented May 3, 2023

Great work,
Some suggestion : Add Event's when open the viewer ,
and close,
and event when click the image in the viewer

Continue working in flet

@icstos
Copy link

icstos commented Dec 1, 2023

Hello, is this ready to use now? I can't find it anywhere in the code or documentation.

@FeodorFitsner
Copy link
Contributor

Nope, it's still an open PR. It should be made as a control as others.

@Skquark
Copy link
Author

Skquark commented Dec 2, 2023

Nope, it's still an open PR. It should be made as a control as others.

I had already rewritten it as a Control in the other PR #960 which got closed, and this one stayed open instead. Sorry if there was misunderstanding, it's been done since January. Been waiting for it to get merged since then, but grew a bit stale. Probably needs minor adjustments and review on your end since the fork was old and has conflicts now and I haven't been able to officially test it. Would still like to see it finalized though..

@yunguangli
Copy link

will this be in the new release soon?

@Skquark
Copy link
Author

Skquark commented Dec 18, 2023

I created another Pull Request at #2248 with the updated commit. Hopefully I did everything right and there's no problems with it, but if you see anything to be fixed or improved, please do. I'm not able to compile and test it, but pretty sure it'll work.

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