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

libdisplay: remove redundant GRASS_NOTIFY #2135

Closed
wants to merge 1 commit into from

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Jan 28, 2022

This addresses one of the '-Wunused-result' compiler warnings reported in #2128.

The warning is caused by a call of system(cmd) upon display driver closure, where cmd is set by the environment variable GRASS_NOTIFY (if set). The warning in itself is not critical.

This PR is a suggestion to address this warning by removing this GRASS_NOTIFY "hack" or whatever it is.
This is the only place in GRASS sources where GRASS_NOTIFY is used at all.

The big question is, is this GRASS_NOTIFY mechanism used by anyone, or any platform?
My research into the motivation of its introduction to GRASS 7.0 [1], or of any of its use, turned up empty. I assume there was a good reason for this in the transition to G7, but now? The GRASS_NOTIFY environment variable is also not documented.

[1] initially committed to trunk: https://trac.osgeo.org/grass/changeset/32602/grass/trunk/lib/raster/raster.c

@nilason nilason added this to the 8.2.0 milestone Jan 28, 2022
@nilason nilason added bug Something isn't working C Related code is in C labels Jan 28, 2022
@nilason nilason mentioned this pull request Feb 4, 2022
21 tasks
wenzeslaus
wenzeslaus previously approved these changes Feb 24, 2022
@wenzeslaus wenzeslaus self-requested a review February 24, 2022 02:22
@wenzeslaus wenzeslaus dismissed their stale review February 24, 2022 02:23

I disagree with myself.

@wenzeslaus wenzeslaus removed their request for review February 24, 2022 02:23
@wenzeslaus
Copy link
Member

It is there as one of the many attempts to improve the rendering workflow. It is not documented and not used in the code and there is no motivation in the commit message.

However, I double checked and some or all (?) usage of ximgview and wxpyimgview can be broken if removed. See:

A suitable (although rather primitive) viewer is available as visualisation/ximgview. This will refresh its display from the file either at regular intervals, or when it receives SIGUSR1. If the environment variable GRASS_NOTIFY exists, its value will be executed via system() whenever a d.* command completes, so you can use e.g.:

export "GRASS_NOTIFY=kill -USR1 `pidof ximgview`"

[GRASS-dev] Shell scripts, Glynn Clements, Dec 2009 (https://narkive.com/9nUlmRam.12)

SIGUSR1 is used in ximgview and wxpyimgview. wxpyimgview could probably use watchdog as wxGUI is using but it may not be the same.

@nilason
Copy link
Contributor Author

nilason commented Feb 24, 2022

I did see that single mention in ML, but that’s all. I’m still convinced this is a hack/debug/test thing. Besides, it is not particularly safe to pass an env var unchecked to a system call.

@wenzeslaus
Copy link
Member

I'm for removing it, but doesn't that mean we need to remove SIGUSR1 from in ximgview and wxpyimgview, too? Then the question is how they will update? Both ximgview and wxpyimgview stand somewhat outside of the standard system as alternatives when the main GUI or d.mon wx* does not suite your needs. It is little hard to make decisions about these. I think it is fair to say that they are ad hoc additions. This might be something to discuss on grass-dev.

@nilason
Copy link
Contributor Author

nilason commented Mar 2, 2022

This might be something to discuss on grass-dev.

I agree, just initiated.

@nilason
Copy link
Contributor Author

nilason commented Dec 15, 2022

Closes this in favour of #2705.

@nilason nilason closed this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants