-
Notifications
You must be signed in to change notification settings - Fork 580
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 image to custom circuit appearance #1924
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be at least reviewed/tested also from @BFH-ktt1. The suggested changes look sensible to me. It would be nice if you could provide also some documentation and use case / screenshot of this functionality to demonstrate its usage.
IMHO, it is problematic if images get downloaded dynamically during loading of circuit files. It could cause hard to handle security risks. Restricting it to only local files may be a bit safer. Ideally, the image should be saved as part of the circuit file. In any case, some security risks remain due to manipulated image files causing buffer overflows and similar things. |
Thanks anyway for your explanations and illustrative screenshots. I can see a certain value in this functionality, but I am not sure whether it is worth the risk. Therefore, I would like feedback by other regular contributors to this project, i.e., @BFH-ktt1, @davidhutchens, @MarcinOrlowski, @R3dst0ne, @TheEvilSkeleton. |
Thanks for your fast feedback!
I imagine some solutions for this :
I would like to know your opinion about these ideas. |
I agree with @maehne about the use of network URLs to load files during startup. We have not used networks for any functionality, and I don't think loading images is a reason to start now. At the least, this would cause .circ files to behave differently if no network is available. At worst, it might have security implications. Therefore, I would reject your first three bullet points. I also note that for Mac users, we actually say in the documentation that you can deny network access to our program without causing any problems. That leaves using local files to load the original image and either linking to the local file or saving the image in the .circ file. We currently have the ability to link to libraries and other .circ files in our local file system. So linking to image files would not be much different. As with the libraries, it is best to use relative paths so a directory can hold the .circ files and images that are linked. This would provide easy sharing of the package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the raised security concerns. Personally, I would tend to embed the images inside the circuit file. However, it is important that they get encoded in a way that the circuit files remain pure text XML files.
The most common way to encode an image in text is base64, but having images in circ files will increase it size. Even if we resize it, an 640x480 random color JPG image takes about 0.5MB in base64. |
I'd not worry the size much, frankly speaking. Since feature is optional to use, if size is a concern, one can just not use it. |
I would be OK with base64. Is 640 x 480 pixels or bigger really the size of images, you would typically assign to a circuit symbol? In any case, base64 doesn't prevent using a compressed image format such as PNG or JPEG. In any case, a vector format -- ideally SVG would be preferred to guarantee good quality. Maybe, the intent/requirements for this enhancements should be first analysed/discussed a bit more in detail before deciding where/how to store the image data. |
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v2...v3) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3 to 4. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v3...v4) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Max 10 based FPGA
Changes :
|
# Conflicts: # CHANGES.md
@Toshayo: Thanks for this additional work on your PR to incorporate our feedback. Could you please provide a small test case using your feature in form of a ZIP archive including a CIRC file + the used image(s)? This would facilitate testing on our side. |
Here is same example as a little higher: logisim_image_example.zip |
Added image shape to custom circuit appearance. The image has one attribute: the URL. It is downloaded and shown when the circuit is rendering.
I've also added an option to disable the image download.