| [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #3213
Application: | FLTK Library |
Status: | 1 - Closed w/Resolution |
Priority: | 1 - Request for Enhancement, e.g. asking for a feature |
Scope: | 3 - Applies to all machines and operating systems |
Subsystem: | FLUID |
Summary: | External Editor for Fluid. |
Version: | 1.3-feature |
Created By: | matt |
Assigned To: | greg.ercolano |
Fix Version: | 1.3.4 (SVN: v11816) |
Update Notification: | |
Trouble Report Files:
|
#1 | greg.ercolano 17:23 Jun 17, 2016 |
| editor.patch 49k | |
|
#2 | greg.ercolano 17:53 Jun 26, 2016 |
| editor2.patch 51k | |
|
#3 | greg.ercolano 23:38 Jul 07, 2016 |
| erco-fluid-ext-editors-v1.patch 41k | |
|
#4 | greg.ercolano 02:56 Jul 09, 2016 |
| erco-fluid-ext-editors-v2.patch 51k | |
|
#5 | greg.ercolano 10:48 Jul 09, 2016 |
| erco-fluid-ext-editors-v3.patch 49k | |
|
#6 | greg.ercolano 02:14 Jul 10, 2016 |
| erco-fluid-ext-editors-v4.patch 70k | |
|
#7 | greg.ercolano 17:10 Jul 11, 2016 |
| erco-fluid-ext-editors-v5.patch 57k | |
|
#8 | greg.ercolano 11:15 Jul 13, 2016 |
| erco-fluid-ext-editors-v6.patch 63k | |
|
#9 | AlbrechtS 07:20 Jul 15, 2016 |
| update_fluid.patch 4k | |
|
#10 | AlbrechtS 07:27 Jul 15, 2016 |
| fix_mingw_build.patch 2k | |
|
#11 | AlbrechtS 07:35 Jul 15, 2016 |
| fluid-ext-editors-v7.patch 28k | |
|
#12 | AlbrechtS 08:43 Jul 15, 2016 |
| fluid-ext-editors-v7a.patch 67k | |
|
#13 | greg.ercolano 10:49 Jul 15, 2016 |
| fluid-ext-editors-v7b.patch 66k | |
Trouble Report Comments:
|
#1 | matt 04:40 Apr 04, 2015 |
| Should at least prevent multiple edits on same code. Should probably use threads to fire off editor, and when editor completes, apply changes using a global lock. Tree window should still be scrollable while edit(s) are in progress. On close or compile, should warn if any unsaved editors are still open. (Multiple editors open on different code should be OK, as long as they're all closed before Alt-G, Alt-S, etc) Use temp files that are named appropriately to the code being edited, so that the editor's window title gives a hint which code is being edited. I think mingodad has a patch already in STR#2794, not sure if he implemented it this way. | |
|
#2 | engelsman 16:53 Apr 08, 2015 |
| I just saw there's also a very old fluid_hack at http://www.fltk.org/links.php?V54+Q | |
|
#3 | greg.ercolano 17:23 Jun 17, 2016 |
| Adding Muhammad Muquit's code here as a patch against 1.0.11 so one can see what's going on. | |
|
#4 | greg.ercolano 17:53 Jun 26, 2016 |
| Hmm, noticed I had the src/dest swapped when I made "editor.patch" (File #1), old code was showing up as '+', and new code as '-'.
Posting new "editor2.patch" (File #2) which has the patch the right way around; old code is '-', new code is '+'. | |
|
#5 | greg.ercolano 19:13 Jun 26, 2016 |
| Agree with most of Matt's comments in #1.
One thing:
I'm not sure we have to make sure all open editors are closed to use Alt-G/Alt-S, etc.
Why can't we just leave fluid-invoked external editors open at all times, and just suck in whatever code is in those live editor files during builds (Alt-X/Alt-G) or saves (Ctrl-S, Shift-Ctrl-C).
This would mean perhaps we wouldn't even need threads or locks.. we just have to make sure all editors are closed before quitting fluid, which I think would just be process management (e.g. checking for child processes), easy to do across unix + windows, and no threads.
Perhaps I'm missing something, but I think that would maybe improve usability, as then one can operate pretty much just like normal; with multiple edits open at all times, without having to loose e.g. cursor positions in the code between builds, etc.
Comments? | |
|
#6 | greg.ercolano 23:38 Jul 07, 2016 |
| Attaching a v1 patch that allows external editors in fluid, proof of concept regarding being able to manage multiple editors open, and that they can remain open during Save / Write Code operations.
Currently linux only; not tested on Mac (though it might work), and no code yet for Win32.
You should be able to apply the patch to svn current (r11800), e.g.
patch -p0 < erco-fluid-ext-editors-v1.patch
To configure the external editor:
1) Open Edit -> GUI Settings 2) Enable "Use external editor" 3) Set the "Editor Command" to e.g. "gvim -f"
The editor command must:
(a) open its own GUI window, and (b) must not background itself (should remain in "foreground")
You should then be able to open an .fl file, double click on any methods/functions that have "Code" (New->Code->Code) assigned, and it should open in the editor.
You should be able to make changes in the editor and save them, and those changes should be seen inside fluid within a second or two of the save:
a) Tree should show first line of code, whatever it is b) Any open "Source Viewer" with "refresh" enabled should auto-update as well c) File -> Save should work even with editors open d) File -> Write Code should work even with editors open e) File -> Save should be activated (not grayed out) whenever an edit is "saved"
FEATURES:
> There can be multiple editors open at a time (one for each "Code" item)
> The editor can remain open even when hitting "Save" or "Write code"
V1 PATCH CAVEATS:
> Currently v1 patch is linux only
> Tested only with "gvim -f" as the editor. Works with "gedit" too, didn't try others
> Boundary cases not tested e.g. if editors left open when new .fl loaded, or Code deleted. When testing this patch, close the editors first before exiting or loading new .fl files. Handling these cases cleanly are a WIP. | |
|
#7 | greg.ercolano 00:02 Jul 08, 2016 |
| Here's a quick video demoing the v1 patch: http://seriss.com/people/erco/fltk/tmp/fluid-ext-editor-demo-v1.mov ..which shows:
> The new "GUI Settings" for configuring the editor command/mode > Opening a new .fl file, and opening two code windows in GVIM > Making changes in GVIM and seeing realtime changes in tree > Try to re-open already open code windows (warning dialog posted) > Enable "Show source code" viewer w/autorefresh, see realtime changes from GVIM > Closing and re-opening code editor windows > Show how "File -> Save" changes from grayed out to enabled when edits are made > Disabling external editor to revert to normal fluid code editing | |
|
#8 | dtopham 23:13 Jul 08, 2016 |
| Thank you for making this patch available. So far I have only built stable releases. (Most recently to enable cairo support.)
I downloaded the r11800 code and applied the patch (no errors). But building I encountered this unfamiliar error:
./configure --enable-cairo configure: error: cannot run /bin/bash ./config.sub
Do you have any suggestions?
config.sub is there and with x permissions in the misc directory | |
|
#9 | greg.ercolano 02:56 Jul 09, 2016 |
| Attaching v2 patch to address some of the boundary issues. | |
|
#10 | greg.ercolano 03:01 Jul 09, 2016 |
| dtopham: I don't recognize it either.
So as not to clutter this STR, suggest taking the question to fltk.general if the following doesn't work.
Error seems unrelated to the patch; suggest trying again with clean r11800 and first verify /that/ builds before applying patch, then do a 'make clean; make' rebuild. | |
|
#11 | greg.ercolano 10:48 Jul 09, 2016 |
| Tested on OSX with "mvim -f" (Mac version of gvim) To install mvim, see: http://stackoverflow.com/questions/21012203/gvim-or-macvim-in-mac-os-x Specifically: Step 1. Install homebrew from here: http://brew.sh Step 2. Run: brew update Step 3. Run: brew install vim && brew install macvim Step 4. Run: brew link macvim Be sure /usr/local/bin is in your PATH.
Attaching v3 which removed some excess bookkeeping code that turned out to be unneeded (L_editors[], etc), some code + comment cleanup/simplification.
Wanted to tighten up code before I start on the Win32 version of the mods, which will be next. Want to actually close this out + commit before I move on to any other STRs. | |
|
#12 | dtopham 20:51 Jul 09, 2016 |
| Thanks Greg, You are correct; the problem I experienced (config.sub), was not an issue with the patch. It appeared to be an issue with building r11800 (which seems to require automake/autoconf) and building 1.3.3 stable (which does not). When I got beyond that I could test the features on my Debian 8 system. All worked as shown in the video (very helpful). However I had been expecting it to also work with the callback area of widgets (such as a button). I used geany as my external editor, but could not see how to invoke it for callbacks. Is that possible? -Dave | |
|
#13 | greg.ercolano 02:08 Jul 10, 2016 |
| dtopham: Yes, I suppose it could also handle the callback code area. I haven't looked into it. As you've seen, currently only only New -> Code -> Code is supported for the external editor. | |
|
#14 | greg.ercolano 02:14 Jul 10, 2016 |
| Attaching v4 patch which includes Win32 support. So now supports all three platforms.
Includes an updated ide/VisualC2010/fluid.vcxproj that includes the correct files for building in the VS 2010 IDE. (I think I used 2012 VSE)
Tested with gvim74.exe as the external editor. Overall behavior is the same with Linux, OSX and Windows.
This is the first pass at the win32 code; created the patch once it seemed to be working. Will give the win32 code a good once over tomorrow. | |
|
#15 | greg.ercolano 17:10 Jul 11, 2016 |
| OK, attaching a second pass code cleanup/simplification of win32+unix code as v5.
I'm thinking this is pretty close to committable. I'd say safe to use this for code review.
* * *
Summary of changes:
fluid/Fl_Type.h -- hooks for code writing and editor mod detection fluid/Fl_Type.cxx -- mods for writing code fluid/Fl_Function_Type.cxx -- hooks to open external editor on double click, write hooks fluid/fluid.cxx _ -- mods for timer checks, close editors on exit, new preference globals fluid/alignment_panel.fl \ fluid/alignment_panel.h >-- mods for new config options to configure editor command + on/off flag fluid/alignment_panel.cxx _/ _ fluid/ExternalCodeEditor_WIN32.h \__ new external editor class (windows implementation) fluid/ExternalCodeEditor_WIN32.cxx _/ _ fluid/ExternalCodeEditor_UNIX.cxx \__ new external editor class (unix implementation) fluid/ExternalCodeEditor_UNIX.h _/ fluid/Makefile -- mods to build new ExternalCodeEditor class files ide/VisualC2010/fluid.vcxproj -- mods to build new ExternalCodeEditor files | |
|
#16 | greg.ercolano 11:15 Jul 13, 2016 |
| Attaching erco-fluid-ext-editors-v6.patch which has IDE + CMake mods so that the external editor mods will build with all IDEs and makefile techniques:
ide/VisualC2008 -- modified, build tested with VS2012 ide/VisualC2010 -- modified, build tested with VS2012 ide/VisualC6 -- modified, build tested with VS 7.net ide/Xcode4 -- modified, build tested with Xcode7 fluid/CMakeLists.txt -- modified, build tested with cmake 3.5 on linux
I didn't test mingw builds with configure or cmake, and didn't test cmake with OSX.
Would like to commit this patch. Might be easier to commit it than to have folks apply the patch + test.
Requesting vote from devs to accept. | |
|
#17 | AlbrechtS 07:20 Jul 15, 2016 |
| Intermediate test status and fixes:
Cygwin (Posix/X11 build) worked, but I was able to make fluid hang after starting the editor (`notepad++ -multiInst`: -multiInst prevents backgrounding), probably with a bad commandline or something like that. The editor process seemed to crash, but fluid didn't recover from that (it hung). I needed to kill the fluid process. Still needs testing, I'll try to find and describe a reproducer. OTOH, using (Cygwin) vi as editor worked.
The sources created from the new fluid files seemed to have been created with an older version of fluid. This could be fixed easily, see attached file update_fluid.patch. This patch must be applied after the v6 patch. | |
|
#18 | AlbrechtS 07:27 Jul 15, 2016 |
| MinGW build didn't work OOTB, but I found fixes. Errors and warnings:
=== making fluid === Compiling ExternalCodeEditor_WIN32.cxx... ExternalCodeEditor_WIN32.cxx: In function 'int terminate_app(DWORD, DWORD)': ExternalCodeEditor_WIN32.cxx:108:11: warning: unused variable 'dwRet' [-Wunused-variable] DWORD dwRet ; ^ ExternalCodeEditor_WIN32.cxx: In member function 'int ExternalCodeEditor::handle_changes(const char**, int)': ExternalCodeEditor_WIN32.cxx:189:32: error: 'GetFileSizeEx' was not declared in this scope if ( GetFileSizeEx(fh, &fsize) == 0 ) { ^ ExternalCodeEditor_WIN32.cxx: In function 'int save_file(const char*, const char*, FILETIME&, LARGE_INTEGER&)': ExternalCodeEditor_WIN32.cxx:360:38: error: 'GetFileSizeEx' was not declared in this scope if ( GetFileSizeEx(fh, &file_size) == 0 ) { ^ ExternalCodeEditor_WIN32.cxx: In member function 'int ExternalCodeEditor::start_editor(const char*, const char*)': ExternalCodeEditor_WIN32.cxx:405:28: warning: passing NULL to non-pointer argument 5 of 'BOOL CreateProcessA(LPCSTR, LPSTR, LPSECURITY_ATTRIBUTES, LPSECURITY_ATTRIBUTES, BOOL, DWORD, PVOID, LPCSTR, LPSTARTUPINFOA, LPPROCESS_INFORMATION)' [-Wconversion-null] &pinfo_) == 0 ) { // process info ^ ExternalCodeEditor_WIN32.cxx:405:28: warning: passing NULL to non-pointer argument 6 of 'BOOL CreateProcessA(LPCSTR, LPSTR, LPSECURITY_ATTRIBUTES, LPSECURITY_ATTRIBUTES, BOOL, DWORD, PVOID, LPCSTR, LPSTARTUPINFOA, LPPROCESS_INFORMATION)' [-Wconversion-null] make[1]: *** [ExternalCodeEditor_WIN32.o] Error 1
----------------
Note: dependencies are NOT set correctly in autoconf/make build. Changing one of the *_WIN32.{cxx|h} files did not recompile the file. Likely needs manual dependency editing in fluid/Makefile for Windows, since `make depend` won't do it for us. :-(
Note2: make depend should be executed (on a Linux system) since we're likely out of sync anyway.
Fixes are in attached file fix_mingw_build.patch. Still needs more tests... | |
|
#19 | AlbrechtS 07:35 Jul 15, 2016 |
| Notes concerning the MinGW patch:
I compiled with lots of warnings enabled (unused variable dwRet).
CreateProcess() used two pointers instead of variables. I guessed that we need 0 and FALSE, resp.. Maybe that's wrong. However, this should be fixed for all Windows compilations, it's not MinGW related.
MinGW's default is a lower WINVER number, hence we need to "fix" this to get the declaration of GetFileSizeEx().
I'm uploading the total patch in fluid-ext-editors-v7.patch. | |
|
#20 | AlbrechtS 07:38 Jul 15, 2016 |
| One more observation/question so far: The Xcode patch introduces:
MACOSX_DEPLOYMENT_TARGET = 10.10;
Is this correct/appropriate? I have no idea, but I suspect this should not be there. | |
|
#21 | greg.ercolano 08:19 Jul 15, 2016 |
| Hmm, you might want to recheck your v7 patch; it doesn't seem to have any differences from v6 other than:
o The ExternalCodeEditor* files are absent.. be sure to 'svn add' them before doing the svn diff so that they're included in the diff
o There seems to be unrelated patches to the fluid/about_panel.cxx and fluid/print_panel.cxx?
Otherwise, there don't actually seem to be any changes relative to v7; perhaps it's a diff of the wrong code base?
I just diffed v6 / v7 patches to see what changed, and after rearranging the order of the files in the v6 patch to match v7's, the code seemed all the same..? Only the rev#s in the diff headers were different -- 11804 vs 11810.. which maybe explains the unrelated fluid panel mods. | |
|
#22 | AlbrechtS 08:24 Jul 15, 2016 |
| More test results (Windows):
Using notepad++ under Windows works. Interesting: no quoting needed for executable file in path with spaces.
FYI: This is what I used with notepad++ (under Windows 8.1): "C:\Program Files (x86)\Notepad++\notepad++ -multiInst"
Note: enter w/o quotes in the "Editor command" field in GUI settings. Works well.
notepad++ shows correct line endings, because it understands bare linefeed line endings, but ...
Using 'notepad' as editor (enter just "notepad" w/o quotes) doesn't show the correct line formatting. Under Windows the temporary file _should_ be written with cr/lf, i.e. using "text mode". I don't intend to fix this now, awaiting response. Greg, I'm sure you'll find that piece of code easier than I could do. AFAICT using text mode under Linux/UNIX doesn't do any harm, so there is probably no need to use platform specific code here. | |
|
#23 | AlbrechtS 08:27 Jul 15, 2016 |
| Oh, and I forgot: The previous tests under Windows were done after building the ide/VisualC2010 project with Visual Studio 2015 after "converting" the projects and running "demo .. fluid", so this seems to work well. | |
|
#24 | greg.ercolano 08:30 Jul 15, 2016 |
| @Comment #17: cool about getting at least cywin + vi to work. Not sure what's up with the "notepad++ -multiInst".. did you try without the multiInst flag? It might work OK; Windows is weird in that e.g. when a windows process appears to background, it doesn't loose connection with CreateProcess().. so you might not need that flag on Windows.
Regarding fluid versions, note my patches are against r11804; see the rev#s in the v6 patch, e.g.
Index: fluid/CMakeLists.txt =================================================================== --- fluid/CMakeLists.txt (revision 11804) <-- +++ fluid/CMakeLists.txt (working copy)
@Comment #20: Yes, the deploy target /was/ 10.6, but on my Xcode7/10.10 box, it wouldn't run the 10.6 deployment, saying 10.10 was "too old", lol. I think Xcode7 has a bug that thinks .10 is less than .6 (alphabetically?) So I left it as 10.10, mostly out of frustration, but yes, should probably leave it at 10.6 in the final, as presumably they'll fix that bug. | |
|
#25 | AlbrechtS 08:43 Jul 15, 2016 |
| @ #21: You're right, I missed 'svn add' for the new files. I worked with git primarily (it's much easier for me to push and pull commits between my Linux VM, the Windows host, and another Windows/Linux PC).
Please take a look at attached file fluid-ext-editors-v7a.patch.
The "unrelated" patches to the *panel source and header files are from using the latest fluid to generate these files from their .fl files. I mentioned this also in my comment #17. It's really not _directly_ related (the code works w/o these patches, and I did not change any source (.fl) files), but it should be fixed anyway, because this is "current" fluid code generation. | |
|
#26 | greg.ercolano 08:45 Jul 15, 2016 |
| @comment #22: I'm curious about spaces being honored in the path w/out quotes, as that /might/ mean command line flags /aren't/ being honored. But just to clarify something, you mentioned you used: "C:\Program Files (x86)\Notepad++\notepad++ -multiInst" If you're trying to test with quotes, I'm thinking that should instead be: "C:\Program Files (x86)\Notepad++\notepad++" -multiInst ..quotes around just the executable path, not the flag? Try that. Or maybe CreateProcess is auto-detecting the executable, and realizing it can honor any spaces after that?
Regarding notepad + bad line endings, yes.. that is 'normal'; notepad sucks ;)
I noticed fluid generates inconsistently generates crlf vs. lf endings when you switch editing in fluid win32 <-> unix. That behavior is unchanged with the external editor mods, and it does come up, even when only using fluid's built in editor.
Note my external editor code uses CreateFile/WriteFile/ReadFile which has no text vs. binary mode; it just writes the data unmodified. The text v. binary is only in the win32 fopen stuff, which I chose not to use, to ensure there's no weird permissions/acls/file locking during write operations (I enable SHARING during write), so the editor doesn't get confused.
I didn't try to 'fix' that, but that might be something that should be fixed in a separate STR.
@Comment #23: good..! And I most appreciate testing/getting mingw to work, as I currently don't have a way to build+test with that. | |
|
#27 | greg.ercolano 09:04 Jul 15, 2016 |
| OK, just checked v7a.
So mainly the changes wrt v6 are:
1) Some CreateProcess() args were changed: NULL,NULL -> FALSE,0 2) Got rid of unused dwRet variable
..which both seem good to me.
The macro mods to get GetFileSizeEx() working on MinGW is interesting. Glad you figured that out ;) I might have switched back to GetFileSize() (without the 'Ex') just to avoid the issue. I could do that BTW; the only reason to use 'Ex' IIRC is the data type can handle super large file sizes.. but in our use case, file sizes will be small, so using 'Ex' /could/ be avoided. But I think I'll leave it if the macro technique solves it. | |
|
#28 | greg.ercolano 09:08 Jul 15, 2016 |
| OK, so assuming your happy with that Albrecht, I'll go for a commit.
I think the only thing I'll revert is that Xcode deploy target number, making sure it stays 10.6 for fluid instead of 10.10.
Will try to make that happen later today; I really want to close this so the brain cells I've used for this STR are free to be reassigned to other purposes ;) | |
|
#29 | AlbrechtS 09:41 Jul 15, 2016 |
| WRT quoting: The MinGW fluid executable works with and w/o quoting. I don't know why, it's a mystery. I recommend correct quoting (as you suggested, only around the exe file name) to get the best results.
With notepad++ "-multiInst" (i.e. "Multi Instance") is necessary. I tried it w/o, and it seems to work "somehow" (sometimes), but it depends on the fact whether another notepad++ instance is already running. If another instance is active the new instance just sends a message to the running instance and terminates immediately. That's not what we want. It's the same with svn and git commits, BTW. The editor process must stay active for git and svn to see when it is time to read the commit message.
> The macro mods to get GetFileSizeEx() working on MinGW is interesting. > Glad you figured that out ;)
That was easy. Just a little Google'ing to find the correct WINVER. I had to do this before in another case in my software, and (!) we have already similar code in src/Fl.cxx. So I just copied this and removed the #if WIN32 conditional because it was unnecessary here.
Regarding "text" vs. "binary" mode: yes, Windows sucks, but notepad is consistent with Windows and hence "correct". The problem is that other Windows users may use notepad and/or other Windows editors that can't cope with LF only line endings. So if we offer using a platform specific editor we should IMHO also write platform specific temp files.
> OK, so assuming your happy with that Albrecht, I'll go for a commit.
I'm not sure yet. I'd like to try the other IDE's first, but maybe we can fix this later if we have a first commit. Also note that I didn't review any code, except reading the diffs and looking for "strange" things. But if you think the code is good, then go for it...
> ... Xcode deploy target number ...
The patch adds one or more lines with just these deploy target statements I cited in my comment. I believe the best solution would be to use an editor and delete these statements (better than using the IDE so _set_ another one).
Okay, that said, going to test Visual2008 and Visual2010 IDE projects with exactly these two IDE versions, resp. on my other PC. Please wait a moment, I'll be back soon... | |
|
#30 | greg.ercolano 09:59 Jul 15, 2016 |
| @Comment #29: > it depends on the > fact whether another notepad++ instance is already running.
Mmm, yeah, I never encountered that situation with Gvim; one process per edit. But I can see where e.g. notepad++ and probably others like sublime might have that issue. Well, I guess we can only support them if they allow one-process-per-edit sessions.
> other Windows users may use notepad
God help them ;)
IIRC, notepad has no line numbering, and there were versions that couldn't handle files above some small limit (64k?). Its a horrible editor. Wordpad is at least somewhat semi-decent.
> So if we offer using a platform specific editor > we should IMHO also write platform specific temp files.
Mmm -- I think addressing this might be a larger issue that has to be dealt with in fluid's internals
I'd thought about having the tmpfile writer doing crlf translations, but when I realized crlf handling was inconsistent across platforms (i.e. no internal translating was being done, so sometimes ^M's would appear in the line endings of some lines in the .fl and .cxx/h files), I didn't want to try to change that.
But certainly it could convert to crlf's when writing the temp file on windows.. this just means tainting the code, as then editors that can handle/maintain unix style lf-only endings would get usurped.
I think it's a larger issue that should probably be dealt with separately, as I think whatever solution you find for the temp files will have affects on the internal fluid code as well.
>> OK, so assuming your happy with that Albrecht, I'll go for a commit. > > I'm not sure yet. I'd like to try the other IDE's first, but maybe > we can fix this later if we have a first commit.
That's what I'm thinking -- I think it'll be easier to collaborate thru svn than patches at this point. I'm pretty sure all the build platforms should work now, and any that don't we can surely fix as per usual.
I guess I'll have to repeat-apply to the 1.3-porting branch. Is that one cmake only now? Then we don't have to deal with the ide/xcode files, which would be great. I kinda forgot what the 1.3-porting branch addresses.
> But if you think the code is good, then go for it...
I do.. it's "clean", no hacks.
The only things that might be a problem are the issues with race conditions I mentioned, and some small/optional TODO items (grep TODO from the patch)
> Please wait a moment, I'll be back soon...
OK -- I won't do any commits until we sync. | |
|
#31 | greg.ercolano 10:49 Jul 15, 2016 |
| Attaching v7b patch, same as v7a but removed unwanted Xcode mods, just so we're on the same page with that. | |
|
#32 | AlbrechtS 11:13 Jul 15, 2016 |
| FTR: tested (with patch v7a) all Windows IDE's successfully with...
ide/VisualC6: Visual Studio 2008 Express ide/VisualC2008: Visual Studio 2008 Express ide/VisualC2010: Visual Studio 2010 Express ide/VisualC2010: Visual Studio 2015 Express
Note: I did not test the latest patch (v7b) which shouldn't matter. I only tested one open editor at a time.
So far everything looks good, AFAICT. | |
|
#33 | AlbrechtS 11:18 Jul 15, 2016 |
| There is only one issue I'd like to have documented VERY clearly because it _could_ do harm: reaping the editor process(es) when FLTK exits may cause data loss if the user opens another file and edits that file in an editor instance started by fluid. This may be done after closing the fluid temp file or in an editor with tabs even while editing the temp (code) file.
Hence, AFAICT, reaping the editor(s) when the fluid window is closed can really be dangerous. Did you document that fact, or should it be documented? Or should we _not_ do this but let the user close the editor window(s)?
Last, but not least: excellent work, thanks a lot! | |
|
#34 | greg.ercolano 11:24 Jul 15, 2016 |
| @Comment #33, re. closing the editor on fluid exit
Mmm, the reason to close editors for temp files is so that if one has two fluid instances running with open editors and closes one, it should close all windows related to that session to prevent confusion.
I could make that an option in the GUI Settings window; "Close editors when fluid exits: yes/no"
Perhaps a follow up STR for that? I'm not too concerned about this STR's mods, because users don't have to enable the feature, and they can always turn it off if they decide they don't like using it. Default is off. | |
|
#35 | AlbrechtS 11:29 Jul 15, 2016 |
| @Comment #30:
> I guess I'll have to repeat-apply to the 1.3-porting branch.
Yep, otherwise this would be missing in FLTK 1.4.
> Is that one cmake only now?
CMake + autoconf/make. Unfortunately we could not drop autoconf, hence we still have to deal with Makefile's and dependencies (for make).
> Then we don't have to deal with the ide/xcode files, which would be great.
Yep, this is really a big advantage.
> I kinda forgot what the 1.3-porting branch addresses.
Better (multi) platform support (platform separation by virtual "driver" methods), CMake as the main build platform (but still autoconf, as mentioned before), no IDE's, new ABI (all ABI guards have been removed), more virtual methods in the core classes (not yet done) and more to come... | |
|
#36 | greg.ercolano 11:34 Jul 15, 2016 |
| The other thing I could do is perhaps post a warning dialog to force the user to first close the open editors, instead of closing them automatically.
This could either be in place of the auto-close code, or could be a GUI Settings option, e.g.
On quitting fluid, any external editors still open should.. a) Be closed automatically b) Warning dialog that asks user to close them first c) Not be closed.. just leave them open
But again, because of potential confusion with multiple fluid sessions, it might be hard for the user to know which editors need to be closed to affect the session being closed.
In all cases though, fluid would remove the temp files on quit. So editors left open would now be editing a file that has been removed in a directory that has been removed, so the editor wouldn't let them save it without renaming it.. good feedback I guess to prevent data loss.
So I think making a GUI option as a separate RFE might be a good thing to solve this. Want to treat that as a separate feature, just so we can get this STR closed + committed. here | |
|
#37 | AlbrechtS 11:35 Jul 15, 2016 |
| ACK. | |
|
#38 | greg.ercolano 11:41 Jul 15, 2016 |
| ..and regarding Comment #36, I suppose (b) could be the default GUI Setting option: warn the user to close the editors before quitting instead of closing them automatically.
That way no chance of data loss, and anyone that decides that's annoying can change the GUI Settings to (a): close the editors.
And perhaps the GUI Settings could include a note about the various implications of (a), (b) and (c).
With all these possibilities, it's again why I'd like to make that a separate RFE/STR, just so this STR gets closed and the feature made available to those using svn current. | |
|
#39 | AlbrechtS 12:27 Jul 15, 2016 |
| Sounds good, please open that other STR and go ahead. TIA. | |
|
#40 | greg.ercolano 12:54 Jul 17, 2016 |
| Fixed in Subversion repository.
v7b patch applied, closed.
Will open a separate STR for the RFE described in Comments #33-38. | |
|
#41 | greg.ercolano 14:55 Jul 18, 2016 |
| Also applied v7b to the fltk-1.3-porting branch as r11818.
Just had to leave out the ide/* patches, as they don't apply to this branch. No conflicts, just offsets, all checked out.
And "svn add"ed the ExternalCodeEditor*{.cxx,h} files this time ;) | |
|
#42 | greg.ercolano 13:28 Aug 16, 2016 |
| Fixed in Subversion repository. | |
[ Return to Bugs & Features ]
|
| |