-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: main
Are you sure you want to change the base?
Conversation
* 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.
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:
|
Does anyone still working on this? |
Apparently, not, as my pull request has been sitting here for a long time
now. Anyway, have you tried it out (my patched version, that is)? Does it
fix your problem?
…On Feb 26, 2018 14:48, "AforAnonymous" ***@***.***> wrote:
Does anyone still working on this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#33 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXnw273QXCE-nXG28m05Er9SDnlj3VOks5tYsQigaJpZM4JnWyk>
.
|
@bruceravel, could you give this a look over please? I can't test it on windows, but it does look good to me. |
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) |
There was a problem hiding this comment.
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
...?
The code I added is only active when 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 |
In my thoughts, remove all Windows support because I cannot test that environment. |
I was just re-reading my code and it seems there is some kind of safety there. cygwin's
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 That way you don't break anything currently working, and you give poor Windows users a way out. |
As GitHub Actions supports Windows, we could test on Windows.
I think this approach is better than now PR. |
@joaotavora I followed your advice and implemented a |
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. |
I cannot test this on WIndows since I don't have a Windows machine. Setting the optional args functions correctly however.
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. |
@joaotavora I added a Windows section on the README. Does it look OK to you? |
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! |
`window-system'
(gnuplot-make-gnuplot-buffer): Make comint buffer differently when
on windows.