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

Markdown: Update using known GeanyDocument when available #1064

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

codebrainz
Copy link
Member

For whatever reason, in some cases document_get_current() doesn't
return a valid document when it seems like it should, so when updating
the markdown preview from signals where the related GeanyDocument
is available, use that instead of calling document_get_current().

In other cases, continue to use document_get_current() as before.

Closes #1062

For whatever reason, in some cases `document_get_current()` doesn't
return a valid document when it seems like it should, so when updating
the markdown preview from signals where the related GeanyDocument
is available, use that instead of calling `document_get_current()`.

In other cases, continue to use `document_get_current()` as before.

Closes geany#1062
@elextr
Copy link
Member

elextr commented Feb 10, 2021

Do you have any further information about when/why/how document_get_current() is not returning valid docs? Given its used all through Geany its a bit of a worry if its not right.

@codebrainz
Copy link
Member Author

I do not, I just noticed that it was giving invalid/NULL and that I had a valid document pointer already available. Probably one of the callbacks the plugin uses gets triggered early before the document list is fully initialized or something, but I didn't spend much time trying to understand it.

@elextr
Copy link
Member

elextr commented Feb 10, 2021

It can definitely return NULL for lots of reasons, but if it returns a pointer it should be valid since it tests it and returns NULL if not valid.

@@ -192,7 +193,7 @@ static gboolean on_editor_notify(GObject *obj, GeanyEditor *editor,
SCNotification *notif, MarkdownViewer *viewer)
{
if (IS_MOD_NOTIF(notif)) {
update_markdown_viewer(viewer);
update_markdown_viewer(viewer, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
update_markdown_viewer(viewer, NULL);
update_markdown_viewer(viewer, editor->document);

@b4n
Copy link
Member

b4n commented Feb 10, 2021

What your change suggests to me is that you'd encounter cases where document_get_current() doesn't return the document for which the signal was fired for. This sounds weird for the activate/new/open/reload/filetype-set signals you're using, and definitely something that should be fixed if it's indeed not in sync (e.g. the signal is fired before the changes that make document_get_current() return the right value happen) -- unless there is an actual reason for that, but still.

I'm sympathetic to the base idea of using the document for which the signal was fired though. But it really seems like it should be strictly equal to document_get_current().

{
GeanyDocument *doc = document_get_current();
if (!DOC_VALID(doc))
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't simply doc == NULL work here? Do you really get non-NULL doc pointers that have doc->valid == FALSE?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown Plugin Preview not working after starting geany with a file that was already opened the other day
4 participants