Skip to content

Commit

Permalink
Replace expression with DOC_VALID
Browse files Browse the repository at this point in the history 
@elextr I still need the extra 'doc->file_name' check otherwise Geany
crashes if the user has an 'untitled' document in front of them. Not a
typical use case, but I encountered the crash by accident just a couple
days after using the plug-in.
  • Loading branch information
@andy5995
andy5995 committed Feb 20, 2024
1 parent b1ff99c commit 8151d8b
Showing 1 changed file with 1 addition and 2 deletions.
3 changes: 1 addition & 2 deletions pinner.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,8 @@ pin_activate_cb(GtkMenuItem* menuitem, gpointer pdata)
(void)pdata;

GeanyDocument* doc = document_get_current();
if (!(doc && doc->is_valid))
if (!DOC_VALID(doc))
return;

// See https://github.com/geany/geany/pull/3770 for more info on
// Why this check is necessary even after checking if doc == NULL
if (doc->file_name == NULL)
Expand Down

5 comments on commit 8151d8b

@elextr
Copy link

@elextr elextr commented on 8151d8b Feb 20, 2024

Choose a reason for hiding this comment

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

Not sure new unsaved documents are that rare, so yeah thats another circumstance you need to handle.

@andy5995
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not sure new unsaved documents are that rare, so yeah thats another circumstance you need to handle.

Ok. My reason for geany/geany#3770 was that I believed it should be handled by the API.

To be more succinct, are you sure you want is_valid to be true for untitled, unsaved docs. And if so, could you explain a little why?

@andy5995
Copy link
Owner Author

@andy5995 andy5995 commented on 8151d8b Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure new unsaved documents are that rare, so yeah thats another circumstance you need to handle.

Ok. My reason for geany/geany#3770 was that I believed it should be handled by the API.

To be more succinct, are you sure you want is_valid to be true for untitled, unsaved docs. And if so, could you explain a little why?

And actually, if you do agree, I think my PR should be done a little differently. Instead of returning NULL from where I do, is_valid should be set to 0 around here maybe https://github.com/andy5995/geany/blob/6d06acffa2ab10a0416e550e4b2ac47a597ebbba/src/document.c (L679)

@elextr
Copy link

@elextr elextr commented on 8151d8b Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To be more succinct, are you sure you want is_valid to be true for untitled, unsaved docs. And if so, could you explain a little why?

DOC_VALID simply means that the pointer is to a GeanyDocument and that it still exists, it hasn't been closed since last time you used it. It says nothing about any other state of the document, eg if it has a filename, that needs to be tested after you are sure its a valid document, otherwise you could be testing rubbish left over after close. IIRC the memory of GeanyDocuments is never re-used so cached pointers will never accidentally point to a different document and so the test of is_valid is always the same document object.

And actually, if you do agree, I think my PR should be done a little differently. Instead of returning NULL from where I do, is_valid should be set to 0 around here maybe https://github.com/andy5995/geany/blob/6d06acffa2ab10a0416e550e4b2ac47a597ebbba/src/document.c (L679)

No.

Perhaps emphasising a couple of concepts might help, a "document" is the in memory buffers and associated metadata, it is not a "file" on disk. It might have an associated name of a disk file for saving to and checking if it changed in background, but it might not. There is absolutely no reason that a memory buffer can't exist and be edited happily without any file associated with it.

Note see the API DOC_FILENAME that can return a pointer that can distinguish "untitled" from real filenames.

@elextr
Copy link

@elextr elextr commented on 8151d8b Feb 20, 2024

Choose a reason for hiding this comment

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

PS also note the difference between doc->file_name which is the display name in UTF-8 and doc->real_path which is the file on disk path probably in the right "encoding" for the user system, maybe, possibly.

Please sign in to comment.