Opened 5 years ago

Closed 3 years ago

#10928 closed bug (fixed)

[Screenshot] Artwork folder is read-only (easy)

Reported by: diver Owned by: nobody
Priority: normal Milestone: R1
Component: Applications/Screenshot Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Since Artwork folder lives in a package now, the Screenshot option to save there should either be removed or non-packaged version of Artwork folder should be added.

Attachments (2)

Change History (11)

comment:1 Changed 4 years ago by pulkomandy

Resolution: fixed
Status: newclosed

Fixed in hrev48507.

comment:2 Changed 4 years ago by diver

Resolution: fixed
Status: closedreopened

Still unable to save there, probably because Screenshot doesn't create B_USER_NONPACKAGED_DATA_DIRECTORY folder on save.

comment:3 Changed 4 years ago by diver

Summary: [Screenshot] Artwork folder is read-only[Screenshot] Artwork folder is read-only (easy)

comment:4 Changed 3 years ago by gbl08ma

Has a Patch: set

comment:5 Changed 3 years ago by gbl08ma

My patch fixes the issue by creating the artwork folder (or any other folder that might be added to the folder selection dropdown in the future) if it doesn't exist.

I have also noticed that when there's a problem saving the file, it just fails silently. I would expect at least a message box informing about the failure, but instead the Screenshot tool just stays open. If one doesn't remember the usual behavior of closing on success, one may be fooled by thinking the file saved sucessfully. But that's probably a subject for another issue.

comment:6 Changed 3 years ago by pulkomandy

Hi, The patch looks good. But there are possible improvements:

1) From the coding guidelines:

Variables start with lowercase letters and use interCapsFormatting.

So "direntry" is not correct. It should be dirEntry or even better, directoryEntry.

2) Testing that the directory exists is not enough. It could be a file or a broken symlink, and in these cases the code below will fail anyway. You could use BEntry.IsDirectory() instead.

3) Creating a temporary BDirectory just for calling CreateDirectory with an absolute path is not very useful. You could use the C function create_directory instead.

comment:7 in reply to:  6 Changed 3 years ago by gbl08ma

Replying to pulkomandy:

Hi, The patch looks good. But there are possible improvements:

1) From the coding guidelines:

Variables start with lowercase letters and use interCapsFormatting.

Oops, I saw this guideline and yet managed to break it.

2) Testing that the directory exists is not enough. It could be a file or a broken symlink, and in these cases the code below will fail anyway. You could use BEntry.IsDirectory() instead.

I read the docs on api.haiku-os.org and I'm still a bit confused over what BDirectory, BEntry, BFile and BPath do, and managed to miss IsDirectory because it comes from BStatable (which in hindsight makes perfect sense, I should have clicked to see the list of all members).

3) Creating a temporary BDirectory just for calling CreateDirectory with an absolute path is not very useful. You could use the C function create_directory instead.

This came from an attempt to stick to object-oriented APIs, and because I couldn't find docs pointing me to create_directory. I looked for a static method in BDirectory to do this (since I agree it doesn't make any sense to create a BDirectory for this) but forgot to look for methods outside of the class. I'm still new to the Haiku API organization style, so forgive me :)

I'll redo my patch (eventually after some minor git wrestling) and reupload.

comment:8 Changed 3 years ago by gbl08ma

I'm not sure if I did the right thing with IsDirectory, but at least now execution no longer continues from there if the entry exists but isn't a directory. In the future this may be extended to give the user a better error message.

comment:9 Changed 3 years ago by pulkomandy

Resolution: fixed
Status: reopenedclosed

Applied in hrev50530, thanks! Feel free to continue with the improved error handling and user alerts, or something else as you wish :)

Note: See TracTickets for help on using tickets.