amelioro / ameliorate

A tool for analyzing debatable problems effectively, collaboratively, and with an open mind.
https://ameliorate.app
MIT License
26 stars 10 forks source link

Allow user to specify resolution when downloading a screenshot #339

Open keyserj opened 9 months ago

keyserj commented 9 months ago

Is your feature request related to a problem? Please describe. I'm happy that I can take a high quality image of my diagram via the screenshot button, but I want to be able to increase or decrease quality and change the dimensions as I want, so that I can get exactly the image I want, perhaps for a desktop background, or a print-out diagram.

Describe the solution you'd like When I click the screenshot button, I want to specify the resolution of the image: image

Describe alternatives you've considered

Additional context The screenshot button is in the MoreActionsDrawer, accessed here: image

Technical ideas

Emmanuel-222 commented 1 month ago

Please assign this to me.

Emmanuel-222 commented 1 month ago

Thank you so much

Emmanuel-222 commented 1 month ago

So how do I start. Should clone this to my system and create a new branch to fix that or what?

Emmanuel-222 commented 1 month ago

Thank you for getting back to me so quickly, but I need some direction on what I will do to start on the fixes of this issue. Please pull off a detailed roadmap for me?

On Mon, 7 Oct 2024 at 17:56, Joel Keyser @.***> wrote:

Assigned #339 https://github.com/amelioro/ameliorate/issues/339 to @Emmanuel-222 https://github.com/Emmanuel-222.

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#event-14544016272, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3IMOEC7K4LNM2MLOBTZ2K4LTAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGU2DIMBRGYZDOMQ . You are receiving this because you were assigned.Message ID: @.***>

keyserj commented 1 month ago

Hey @Emmanuel-222 no problem 🙂

  1. check out the CONTRIBUTING.md file; it's got info about running the project, an overview of the code, and a link at the bottom for the PR process (you'll need to fork this repo and make your changes on your own branch there)
  2. I realize the text in the screenshot is a bit outdated, it says "download screenshot" now - if you search for that text in the repo you'll find the current logic for downloading a screenshot without yet being able to specify the resolution
  3. instead of downloading the screenshot right away, we should be able to open a modal to specify the resolution (then confirming the dialog should trigger the download logic). we're actually doing something similar for renaming a View, so that logic might be able to help you get started. you can find that View-rename dialog on the playground: image

How does that sound? Let me know if you have any more questions.

Emmanuel-222 commented 1 month ago

Thanks, I'll check it out right away!

On Mon, 7 Oct 2024 at 19:25, Joel Keyser @.***> wrote:

Hey @Emmanuel-222 https://github.com/Emmanuel-222 no problem 🙂

  1. check out the CONTRIBUTING.md https://github.com/amelioro/ameliorate/blob/main/CONTRIBUTING.md file; it's got info about running the project, an overview of the code, and a link at the bottom for the PR process (you'll need to fork this repo and make your changes on your own branch there)
  2. I realize the text in the screenshot is a bit outdated, it says "download screenshot" now - if you search for that text in the repo you'll find the current logic for downloading a screenshot without yet being able to specify the resolution
  3. instead of downloading the screenshot right away, we should be able to open a modal to specify the resolution (then confirming the dialog should trigger the download logic). we're actually doing something similar for renaming a View https://github.com/amelioro/ameliorate/blob/8b8c16920c84b2e3ae89abd5c2bc5ffbf52c43b1/src/web/topic/components/TopicPane/QuickViewSection.tsx#L110-L118, so that logic might be able to help you get started. you can find that View-rename dialog on the playground https://ameliorate.app/playground: image.png (view on web) https://github.com/user-attachments/assets/0faa204d-3150-45e3-b2e8-130d8da05ccd

How does that sound? Let me know if you have any more questions.

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#issuecomment-2397601577, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3LFZD6QDYDP35BGSTTZ2LGZJAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJXGYYDCNJXG4 . You are receiving this because you were mentioned.Message ID: @.***>

Emmanuel-222 commented 1 month ago

Hi Joel, I am still on the installation process, but after following the contribution.md file I had an error. Here is a screenshot.

On Mon, 7 Oct 2024 at 19:42, Emmanuel Ogbuefi @.***> wrote:

Thanks, I'll check it out right away!

On Mon, 7 Oct 2024 at 19:25, Joel Keyser @.***> wrote:

Hey @Emmanuel-222 https://github.com/Emmanuel-222 no problem 🙂

  1. check out the CONTRIBUTING.md https://github.com/amelioro/ameliorate/blob/main/CONTRIBUTING.md file; it's got info about running the project, an overview of the code, and a link at the bottom for the PR process (you'll need to fork this repo and make your changes on your own branch there)
  2. I realize the text in the screenshot is a bit outdated, it says "download screenshot" now - if you search for that text in the repo you'll find the current logic for downloading a screenshot without yet being able to specify the resolution
  3. instead of downloading the screenshot right away, we should be able to open a modal to specify the resolution (then confirming the dialog should trigger the download logic). we're actually doing something similar for renaming a View https://github.com/amelioro/ameliorate/blob/8b8c16920c84b2e3ae89abd5c2bc5ffbf52c43b1/src/web/topic/components/TopicPane/QuickViewSection.tsx#L110-L118, so that logic might be able to help you get started. you can find that View-rename dialog on the playground https://ameliorate.app/playground: image.png (view on web) https://github.com/user-attachments/assets/0faa204d-3150-45e3-b2e8-130d8da05ccd

How does that sound? Let me know if you have any more questions.

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#issuecomment-2397601577, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3LFZD6QDYDP35BGSTTZ2LGZJAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJXGYYDCNJXG4 . You are receiving this because you were mentioned.Message ID: @.***>

keyserj commented 1 month ago

Hey @Emmanuel-222 , I don't see a screenshot in your post

Emmanuel-222 commented 1 month ago

Sorry about that, here it is. [image: Screenshot 2024-10-08 114825.png]

On Tue, 8 Oct 2024 at 14:30, Joel Keyser @.***> wrote:

Hey @Emmanuel-222 https://github.com/Emmanuel-222 , I don't see a screenshot in your post

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#issuecomment-2399856795, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3MD2QBMCV2KJZUJZODZ2PNAXAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJZHA2TMNZZGU . You are receiving this because you were mentioned.Message ID: @.***>

Emmanuel-222 commented 1 month ago

I sent it has an attachment, probably didn't load up

On Tue, 8 Oct 2024 at 16:19, Emmanuel Ogbuefi @.***> wrote:

Sorry about that, here it is. [image: Screenshot 2024-10-08 114825.png]

On Tue, 8 Oct 2024 at 14:30, Joel Keyser @.***> wrote:

Hey @Emmanuel-222 https://github.com/Emmanuel-222 , I don't see a screenshot in your post

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#issuecomment-2399856795, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3MD2QBMCV2KJZUJZODZ2PNAXAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJZHA2TMNZZGU . You are receiving this because you were mentioned.Message ID: @.***>

keyserj commented 1 month ago

@Emmanuel-222 I think replying on this thread via email does not allow attachments - seems like others have had this issue https://github.com/orgs/community/discussions/52761. If you want to include the image, you probably need to post it on GitHub directly. If email is easier right now, perhaps you could post an error message in text rather than a screenshot?

Emmanuel-222 commented 1 month ago

Screenshot 2024-10-08 114825

This is it 👆👆

keyserj commented 1 month ago

@Emmanuel-222 I found this WSL issue that seems promising - can you try the steps here https://github.com/microsoft/WSL/issues/5923#issuecomment-1215454084?

In summary, if you run wsl -l before entering WSL, and it shows docker as your default WSL distro, you should use wsl -s <other distro, Ubuntu for me> to set the distro properly, and maybe you'll need to restart your apps that are using wsl for that to take affect. I guess this is a bug that can happen when installing docker?

Let me know if that doesn't fix it!

Emmanuel-222 commented 1 month ago

It fixes it and show another error on running "npm run dev" Screenshot 2024-10-08 210149

Emmanuel-222 commented 1 month ago

@keyserj I fixed it.

Emmanuel-222 commented 1 month ago

Screenshot 2024-10-08 211941

This is the final thing that is stopping me

keyserj commented 1 month ago

@Emmanuel-222 the last line in setupLocal runs docker build -t mock-auth mock-auth/ to build a mock-auth image for mocking login locally. You can run docker image ls to see if the build command successfully created the mock-auth image (latest is just the default tag that's applied). If you don't have that image, try running docker build -t mock-auth mock-auth/ again and see if there are errors.

FYI - commenting here is totally fine, but if you have Discord that might be faster to get through back-and-forth. This is the Discord server for Ameliorate and I'm generally available there for messaging.

Emmanuel-222 commented 1 month ago

@keyserj the project has finally started working. I wanted to ask where the function is in. Which file?

keyserj commented 1 month ago

Refer back to https://github.com/amelioro/ameliorate/issues/339#issuecomment-2397601577

Generally a good way to find where code is, is to search the repo for text that shows up in the UI. That's why I suggested to search for "download screenshot". That should lead you to the button, which has an onClick, and you can trace the code from there. You'll also probably find it helpful to see existing logic to open a modal, which I link to in the comment above.

Emmanuel-222 commented 1 month ago

Okay thank you. Then about login. Still stuck.

keyserj commented 1 month ago

The contributing guide's Running the project mentions that the mock auth service allows you to enter any user/pass to login, and it'll approve you.

Technically for this task, I think you should be able to do everything you need from the playground, but logging in doesn't hurt if you want to explore more.

Emmanuel-222 commented 1 month ago

https://github.com/user-attachments/assets/9633b7e2-ac28-4d69-a024-dfd3e6216b2c

This is what I have done so far. What do you think?

keyserj commented 1 month ago

@Emmanuel-222 functionality seems good, but I'm hoping for something like this: image

I'm also not sure if specifying a quality is necessary - I was envisioning the pixel resolution to indicate both the size and quality of the image.

You should be able to find good guidance for this work by referring to the Technical ideas section of this issue as well as the logic around the "Edit View" button, as that opens to a modal with validation, that should be similar to what you need to do here.

chrome_2024-10-10_10-13-35

Emmanuel-222 commented 1 month ago

Hi @keyserj,

This is what I have worked on so far. What do you think?

https://github.com/user-attachments/assets/7f232792-8c23-468f-b06a-7b1d8ec7afc6

keyserj commented 1 month ago

Hey @Emmanuel-222, that looks pretty cool! I use a tool to take screenshots like this all the time, and it's nice to see what the code might look like.

But I think for the purposes of this issue, this solution has a few significant drawbacks.

First off, to clarify: the width and height that we're currently using for the resolution do not affect what content is in the image, they only affect the size that the image is scaled to. The nodeBounds variable is what determines what's in the image.

So the area being captured for the screenshot is currently unchangeable - it always captures all of the nodes being displayed, fits them within the image, and centers them. That's intentional, so that you don't have to spend effort repositioning or zooming-to-fit your diagram, or only snapping certain parts of it. If you ever want to only capture certain nodes, you can just filter out the other nodes via the app's filtering functionality. Manually capturing an area takes more work to get what you want, and each snapshot you take will likely be slightly different than if you try taking another.

Another downside of the manual capture is that you can only capture a region that fits within your monitor's size. The default resolution is currently larger than my monitor, which enables a higher quality e.g. you can zoom into the screenshot and see more pixels than you can see just looking at the diagram via the app on your monitor.

For these reasons, I think the solution initially suggested by this issue would be better. Namely, this is the solution in the image below, where a modal is provided to enter pixel values if the user desires. image

Emmanuel-222 commented 1 month ago

Can you break this down?, I really don' understand it.

keyserj commented 1 month ago

@Emmanuel-222

Regarding how the code currently works: if you change the lines that hardcode the resolution, say you decrease these from 2560 x 1440 to 1920 x 1080, you'll still get the same nodes in the image, centered in the same spot. But the KB of the image file will be smaller, and the image might look a little more blurry, because there are fewer pixels being used.

This GitHub issue is asking for a modal where you can enter these two numbers. This modal should look and behave very similarly to the Quick View modal in the gif below (but instead of accepting Title, it should accept Width and Height: chrome_2024-10-10_10-13-35

Let me know if there's anything more specific that I can clear up.

Emmanuel-222 commented 1 month ago

Hi @joel, So I think for this one it would be for good user experience that the user maybe not of necessity should only see the resolution(height and width) as they drag the cursor for screenshots. What do you think?

On Tue, Oct 15, 2024, 14:07 Joel Keyser @.***> wrote:

@Emmanuel-222 https://github.com/Emmanuel-222

Regarding how the code currently works: if you change the lines that hardcode the resolution https://github.com/amelioro/ameliorate/blob/b2256c2139a8228cb5e2e6854b2ba7c5ae86ec6f/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx#L70-L71, say you decrease these from 2560 x 1440 to 1920 x 1080, you'll still get the same nodes in the image, centered in the same spot. But the KB of the image file will be smaller, and the image might look a little more blurry, because there are fewer pixels being used.

This GitHub issue is asking for a modal where you can enter these two numbers. This modal should look and behave very similarly to the Quick View modal in the gif below (but instead of accepting Title, it should accept Width and Height: chrome_2024-10-10_10-13-35.gif (view on web) https://github.com/user-attachments/assets/c8d9d003-1938-4f01-bffb-632f30bd1a9a

Let me know if there's anything more specific that I can clear up.

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#issuecomment-2413867831, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3KFO63JEXCPBYFCGYLZ3UHRHAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJTHA3DOOBTGE . You are receiving this because you were mentioned.Message ID: @.***>

keyserj commented 1 month ago

@Emmanuel-222 as I mentioned above, dragging the cursor to take the screenshot is not desirable because:

Do these points make sense?

Emmanuel-222 commented 4 weeks ago

Okay I will do just that. I'm really sorry I'm just replying to this. In Nigeria we had a total blackout for a while due to our energy grid collapsing, so most of what I have been doing has been solely my work. I will get on this as soon as possible. Thank you.

On Thu, Oct 17, 2024, 15:01 Joel Keyser @.***> wrote:

@Emmanuel-222 https://github.com/Emmanuel-222 as I mentioned above, dragging the cursor to take the screenshot is not desirable because:

  • user has to spend effort repositioning the diagram before screenshotting. with the current solution, the diagram is automatically centered in the screenshot.
  • user has to spend effort setting the desired zoom before screenshotting. with the current solution, the diagram is automatically zoomed to fit the size of the screenshot.
  • user can only capture a screenshot that fits within their monitor's size. with the desired solution (entering width x height), you can take a screenshot of any resolution quality you want.

Do these points make sense?

— Reply to this email directly, view it on GitHub https://github.com/amelioro/ameliorate/issues/339#issuecomment-2419637013, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2SYH3JIUEKPFHVKO6PINATZ367KPAVCNFSM6AAAAABPQJUSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJZGYZTOMBRGM . You are receiving this because you were mentioned.Message ID: @.***>

keyserj commented 4 weeks ago

@Emmanuel-222 no worries, sorry to hear about the blackout.