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

Fix gnuplot-mode on Windows #33

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

Conversation

joaotavora
Copy link

  • README.org: Section about Windows problems rephrased.
  • gnuplot.el (gnuplot-echo-command-line-flag): Also check for
    `window-system'
    (gnuplot-make-gnuplot-buffer): Make comint buffer differently when
    on windows.

* README.org: Section about Windows problems rephrased.

* gnuplot.el (gnuplot-echo-command-line-flag): Also check for
`window-system'
(gnuplot-make-gnuplot-buffer): Make comint buffer differently when
on windows.
@joaotavora
Copy link
Author

I realize I should have read #15 before, where people have apparently got it working with a different approach. So take this pull request with a grain of salt. Anyway, this fixes it for me. My problems were:

  • gnuplot couldn't open any windows if called directly (without cmdproxy.exe, that is
  • the value for comint-process-echoes was being incorrectly set in the *gnuplot* buffer, causing it to hang while in accept-process-output, called from comint-send-input.

@no-identd
Copy link

Does anyone still working on this?

@joaotavora
Copy link
Author

joaotavora commented Feb 26, 2018 via email

@jgkamat
Copy link

jgkamat commented Apr 21, 2018

@bruceravel, could you give this a look over please? I can't test it on windows, but it does look good to me.

@conao3
Copy link
Member

conao3 commented Dec 10, 2019

LGTM for me, but I don't have Windows and don't familiar Elisp on Windows.
@tarsius, any thoughts for this PR?

@tarsius
Copy link

tarsius commented Dec 10, 2019

LGTM for me, but I don't have Windows and don't familiar Elisp on Windows.

I don't have Windows either but I maintain some popular packages that do things with paths and stuff, and that are being used by users of Windows...

And that experience tells me that whenever you do something to fix some Windows-specific breakage for one user, then you at the same time break it for another Windows user. That's because there are windows-nt and cygwin Emacsen and they are being used in combination with cygwin and/or non-cygwin tools. And cygwin can be configured in all kinds of crazy was and there are multiple cygwin forks with different. Also Windows now comes with a Linux implementation of sort, that's sure going to make it easier...

For Magit I have my "Windows guy" (who mostly knows what he is doing (I strongly suspect nobody fully knows what they are doing in this domain)) who handles these things for me, but for other packages I am opting for: "we can add your hack, but there must be an option so that users can enable it if only if it actually is broken for them without this".

@@ -570,7 +570,8 @@ The values are
(const :tag "Separate window" window)
(const :tag "This window" nil)))

(defcustom gnuplot-echo-command-line-flag (not gnuplot-ntemacs-p)
(defcustom gnuplot-echo-command-line-flag (and (not gnuplot-ntemacs-p)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested, but this and should be or ...?

@joaotavora
Copy link
Author

And that experience tells me that whenever you do something to fix some Windows-specific breakage for one user, then you at the same time break it for another Windows user. That's because there are windows-nt and cygwin Emacsen and they are being used in combination with cygwin and/or non-cygwin tools. And cygwin can be configured in all kinds of crazy was and there are multiple cygwin forks with different. Also Windows now comes with a Linux implementation of sort, that's sure going to make it easier...

The code I added is only active when (eq window-system 'w32). Does cygwin Emacs respond to that?

Also, I believe that every package should behave more or less the way that Emacs itself handles windows stuff. So assume non-cygwin tools (the Gnuplot executable I was using was the "official one", if I remember correctly. Then give cygwin users a customization option. I.e. don't cater for specific packaging systems, that would be my advice.

That said, I'm not using windows anymore (yay!) and no Gnuplot either, so take this with a grain of salt and tweak the PR all you want!

My 2c

@conao3
Copy link
Member

conao3 commented Dec 10, 2019

In my thoughts, remove all Windows support because I cannot test that environment.

@joaotavora
Copy link
Author

we can add your hack, but there must be an option so that users can enable it if only if it actually is broken for them without this

I was just re-reading my code and it seems there is some kind of safety there. cygwin's shell-file-name shouldn't be cmdproxy.exe in principle. And you can also point gnuplot-program to whatever you want.

In my thoughts, remove all Windows support because I cannot test that environment.

That sounds a bit extreme. Emacs supports windows, so it's silly if one package purposedly doesn't support it. The lowest effort thing here is to add a new variable gnuplot-program-args, defaulting to nil and apply the make-comint function to it.

That way you don't break anything currently working, and you give poor Windows users a way out.

@conao3
Copy link
Member

conao3 commented Dec 10, 2019

That sounds a bit extreme. Emacs supports windows, so it's silly if one package purposedly doesn't support it.

As GitHub Actions supports Windows, we could test on Windows.
BTW, what is the official Windows support? I know there is many difference cygwin and MSYS, but Emacs official page argued MSYS only. https://www.gnu.org/software/emacs/download.html#windows

The lowest effort thing here is to add a new variable gnuplot-program-args, defaulting to nil and apply the make-comint function to it.

I think this approach is better than now PR.

@mtreca
Copy link
Collaborator

mtreca commented Dec 13, 2020

@joaotavora I followed your advice and implemented a gnuplot-program-args customizable variable. If that looks OK to you I can merge #65 and close this PR.

@joaotavora
Copy link
Author

@joaotavora I followed your advice and implemented a gnuplot-program-args customizable variable. If that looks OK to you I can merge #65 and close this PR.

Have you tested on windows? I haven't and I won't since I don't use it anymore. Go and merge the other PR, I guess, but at least show the windows users in the README how to make use of that variable, or link them here at least.

@mtreca
Copy link
Collaborator

mtreca commented Dec 13, 2020

I cannot test this on WIndows since I don't have a Windows machine. Setting the optional args functions correctly however.

Go and merge the other PR, I guess, but at least show the windows users in the README how to make use of that variable, or link them here at least.

Yes I intend to add an up-to-date section to the README.org before pushing by gathering info from this thread and #15. But I honestly don't know if Windows support will ever be a thing.

@mtreca
Copy link
Collaborator

mtreca commented Dec 13, 2020

@joaotavora I added a Windows section on the README. Does it look OK to you?

@joaotavora
Copy link
Author

The README isntructions look fine, though quite more complicated than what I had here, which seems harmless. If you know what you are doing, go for it!

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

Successfully merging this pull request may close these issues.

6 participants