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

accounting for directories in file path #96

Merged
merged 6 commits into from
Mar 23, 2024

Conversation

rachancheet
Copy link

@rachancheet rachancheet commented Mar 1, 2024

Currently Wayshot save screenshots in the current working directory. However this is not ideal as some users like to save screenshots in a particular directory(For example $HOME/Pictures/screenshots).
This PR introduces a --folder flag which allows users to select save directory for their screenshots.

@Shinyzenith
Copy link
Member

I believe this can be done with -f, --file.

@rachancheet
Copy link
Author

rachancheet commented Mar 1, 2024

User will have to manually set a unique file name every time they want to save a screenshot in a particular directory . Folder flag makes it so user can just specify the folder location and file names are UNIX_TIMESTAMP-wayshot.EXTENSION

@Shinyzenith
Copy link
Member

I think this is better solved by having file flag accept paths too. That way we can automatically have the same behavior. Introducing too many flags isn't great UX.

@rachancheet rachancheet changed the title added --folder flag accounting for directories in file path Mar 2, 2024
@rachancheet
Copy link
Author

rachancheet commented Mar 3, 2024

I have added the requested change. Please review.

@Shinyzenith
Copy link
Member

CC: @zubairmh @Decodetalkers

Copy link

@zubairmh zubairmh left a comment

Choose a reason for hiding this comment

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

Aside from some styling changes that can be made, commit is ok

@Shinyzenith
Copy link
Member

Aside from some styling changes that can be made, commit is ok

Can you expand on this? Style looks fine to me.

@zubairmh
Copy link

zubairmh commented Mar 3, 2024

Can you expand on this? Style looks fine to me.

@Shinyzenith
This section could be rephrased

        if pathbuf.is_dir() {
            pathbuf.push(utils::get_default_file_name(requested_encoding));
            Some(pathbuf)
        } else {
            Some(pathbuf)
        }

to something like

        if pathbuf.is_dir() {
            pathbuf.push(utils::get_default_file_name(requested_encoding));
        }
        Some(pathbuf)

might just be feeling picky, either way works

@Gigas002 Gigas002 mentioned this pull request Mar 4, 2024
@rachancheet
Copy link
Author

cc @zubairmh Please review.

@zubairmh
Copy link

zubairmh commented Mar 4, 2024

looks good. lgtm

@Shinyzenith
Copy link
Member

Hi, CI is failing on this PR. Kindly format your patch.

@rachancheet
Copy link
Author

cc @Shinyzenith fixed.

@Shinyzenith
Copy link
Member

Hi, I'd like to merge this, can you resolve the conflicts?

@rachancheet
Copy link
Author

Done. @Shinyzenith

@Shinyzenith
Copy link
Member

Thanks! However, I redid the entire documentation block to be more concise and easy to read.

@Shinyzenith Shinyzenith merged commit 2afa5b0 into waycrate:freeze-feat-andreas Mar 23, 2024
3 checks passed
CheerfulPianissimo pushed a commit to CheerfulPianissimo/wayshot that referenced this pull request Mar 24, 2024
Signed-off-by: Shinyzenith <[email protected]>

---------

Signed-off-by: Shinyzenith <[email protected]>
Authored-by: rachancheet <[email protected]>
Co-authored-by: Shinyzenith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants