waycrate / wayshot

Mirrored at https://git.sr.ht/~shinyzenith/wayshot | screenshot tool for wlroots based compositors implementing zwlr_screencopy_v1
https://crates.io/crates/wayshot
BSD 2-Clause "Simplified" License
108 stars 18 forks source link

accounting for directories in file path #96

Closed rachancheet closed 3 months ago

rachancheet commented 4 months ago

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 commented 4 months ago

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

rachancheet commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

I have added the requested change. Please review.

Shinyzenith commented 4 months ago

CC: @zubairmh @Decodetalkers

Shinyzenith commented 4 months ago

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

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

zubairmh commented 4 months ago

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

rachancheet commented 4 months ago

cc @zubairmh Please review.

zubairmh commented 4 months ago

looks good. lgtm

Shinyzenith commented 4 months ago

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

rachancheet commented 4 months ago

cc @Shinyzenith fixed.

Shinyzenith commented 3 months ago

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

rachancheet commented 3 months ago

Done. @Shinyzenith

Shinyzenith commented 3 months ago

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