View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0008753 | Scribus | Integration | public | 2010-01-29 12:08 | 2016-12-05 20:21 |
Reporter | cezaryece | Assigned To | cbradney | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Platform | Linux | OS | Kubuntu | OS Version | Karmic |
Product Version | 1.5.0svn | ||||
Target Version | 1.5.3 | ||||
Summary | 0008753: Edit image in Gimp - must completely close Gimp to edit new image | ||||
Description | You can open few images in one instance of Gimp. But not invoking it from Scribus. | ||||
Steps To Reproduce | 1. insert graphic frame and load some picture into it 2. select from menu Edit / Edit Image which launch Gimp and load selected image into it 3. close only image not closing whole Gimp 4. try to edit image once more result: warning "Gimp is running", you have to close whole Gimp for next image editing | ||||
Additional Information | In fact I dont want to edit more than one image from Scribus in Gimp, but I want not to have to close whole Gimp for next image edit. On my laptop (which is not very fast machine) closing Gimp take about 2 seconds. And if I have to close it because I have to launch it again is very frustrating. | ||||
Tags | Gimp | ||||
Patch | Yes | ||||
|
can't replicate it on debian testing... works as expected... |
|
As I wrote in bug details: Kubuntu Karmic, Scribus 1.5.svn, Gimp 2.6 |
|
can you check if you have gimp-remote installed? and if yes, set the editor in scribus to gimp-remote instead of gimp? (it's probably not there, but it looks like you're starting gimp as "gimp -n"...) |
|
I have not any such packet as gimp-remote in my distribution... Also I got any gimp-remote executable in /usr/bin. As I read about it, gimp-remote is no longer needed. New Gimp uses D-bus functionality for opening files in one session. |
|
since it's working here (on what i suppose being a very similar system), i guess that the problem is more on the gimp side. one question: can you open an image from the terminal while gimp is already open and close it without closing gimp and reopen it? if not, i think you should check with the gimp folks... |
|
Yes, I can open images from terminal in one session of Gimp. After closing all images Gimp stays open without images (empty window) and I can open other images from terminal. For me it looks like Scribus checks if Gimp is working, and submit warning if yes. Warning is from Scribus! |
|
i "hope" that someone with kubuntu karmic and 1.5svn can reproduce the issue (or not)... i don't think that scribus is checking for gimp being opened or not. and as i wrote: it works correctly on debian testing and has been working reliably on my computer since a long time. i fear a configuration problem on your computer, but i have not way to check. we will have to close the bug if nobody else can reproduce it... |
|
BTW I try to find this feature in code, but it is very, very hard to recognize what is what doing in it. There is no comments and it doesn't help to understand code. I would like to join devel team and help with development Scribus, but how to eat this thousand of code lines without any tips and guidelines? |
|
Just now I discover another interesting thing with this issue. If Gimp is running before invoking it from Scribus all works as expected - I can open image from Scribus many times without any problems. If Gimp starting when Scribus invoke it than I cant open next image in Gimp from Scribus till I close Gimp. For now it is good workaround for this issue. |
|
i can confirm the bug on debian testing: if scribus-ng launches gimp, it won't be able to edit a second image if gimp has not been closed after editing the first image. if gimp is already open, i can edit as many images as i want. |
|
removing the check for the running gimp solves the problem (ScribusMainWindow::callImageEditor() in scribus.cpp: 9104,9109d9103 < if (ExternalApp != 0) < { < QString ieExe = QDir::convertSeparators(imageEditorExecutable); < QMessageBox::information(this, tr("Information"), "<qt>" + tr("The program %1 is already running!").arg(ieExe) + "</qt>", 1, 0, 0); < return; < } my question is: why is the check there? i would also rename "callImageEditor()" as "callExternalEditor()", since it's also used for the latex editor... |
|
I apply ale suggestion to my SVN working copy and works like a charm. Repeat question: why this check is here? I attach patch for removing it for simplify testing task. |
|
scribus.cpp.diff (878 bytes)
Index: Scribus/scribus/scribus.cpp =================================================================== --- Scribus/scribus/scribus.cpp (wersja 15626) +++ Scribus/scribus/scribus.cpp (kopia robocza) @@ -9327,12 +9327,12 @@ } #endif QString imageEditorExecutable=prefsManager->imageEditorExecutable(); - if (ExternalApp != 0) - { - QString ieExe = QDir::convertSeparators(imageEditorExecutable); - QMessageBox::information(this, tr("Information"), "<qt>" + tr("The program %1 is already running!").arg(ieExe) + "</qt>", 1, 0, 0); - return; - } +// if (ExternalApp != 0) +// { +// QString ieExe = QDir::convertSeparators(imageEditorExecutable); +// QMessageBox::information(this, tr("Information"), "<qt>" + tr("The program %1 is already running!").arg(ieExe) + "</qt>", 1, 0, 0); +// return; +// } if (currItem->PictureIsAvailable) { int index; |
|
Confirm bug for * Ubuntu 9.10 (Karmic), Scribus 1.3.7 from Scribus debian repository, and for * Ubuntu 10.10 (Maverick), Scribus 1.3.8.dfsg-1.1 from Ubuntu repository. Behavior as in http://bugs.scribus.net/view.php?id=8753#c23192 It'd be great if this annoying bug could be fixed (and it would probably be a good idea to document the rationale). |
|
To explain the hesitation in applying this patch to svn 1. We have to remember that while gimp is the default, it can be changed to any number of other image editors. I know there are folks who want Photoshop on Windows or OSX, along with others who might prefer Krita or Showfoto depending on their work flow. 2. When the external app is already running, the patch would create a new instance of the QProcess without deleting the old one. Not good 3. We have no platform independent way of detecting if gimp is running. So solving the issue is a bit more complex. |
|
1. OK, so patch should be platform depended and in preferences able to switch warning on/off depend how it works with current system/editor 2. I must investigate what you wrote 3. why we should check this? launching Gimp (or other editor) every time we have to edit image is wasting of time for application`s initializing - bit irritating with few images. Workaround is launching Gimp outside of Scribus mechanism - no warnings, no additional time for launch, but why it is not possible from Scribus. Is launching of outside application from Scribus generating new QProcesses every time? |
|
i would not make a platform dependent patch. but it would be helpful to know why this check is here. i mean: if an application can't cleanly handle multiple calls on an existing instance, why is this a scribus problem? gimp can perfectly open a new image while it's already running. if an application can't do it, we should not bother: having a new thread is what the devs of that application wanted. imo. in my eyes, the real question is: why is scribus care about it? are we not overcareful? i would suggest that we find out where the problem is and solve it, not write workarounds. the current situation is a bad one and has to be fixed, since -- afaik -- makes a well behaving application unusable with scribus. |
|
Same issue exists on Windows platform as well. Is there an equivalent scribus.cpp file to try the workaround posted above with? |
|
Same happens with today 1.5 svn on ubuntu 12.04 : - close Gimp, launch Scribus, - open Extra > Manage Images - click an image thumbnail, - choose "Image tools" pane, - click "edit image" button It launches gimp. - edit image, close image window leaving gimp running - choose another image, click "edit image" Scribus displays "Gimp is allready running" error message. That's embarassing. |
|
Can reproduce jluc's 0008753:0030132 William, might you have moment to take a look at this issue? The core devs are pretty busy getting 1.5.1 ready for release. Thanks in advance! |
|
Jurajf, would you kindly weigh in on this issue? Thanks in advance! |
|
The "Gimp is already running" message still happens for me with the current SVN source. The code is in scribus/scribus.cpp in ScribusMainWindow::callImageEditor(). Scribus saves the created QProcess() in ScribusMainWindow::ExternalApp, so Scribus can handle only one external application at a time. If you simply overwrite ExternalApp with a new QProcess(), you lose access to the previous (but still running) QProcess(). That is why removing the test for ExternalApp is not good, as suggested in comments https://bugs.scribus.net/view.php?id=8753#c23215 and https://bugs.scribus.net/view.php?id=8753#c24788 I think that the solution is changing ExternalApp from a variable to some type of collection, maybe QLinkedList, that can track all of the open processes. On Linux, if gimp is open, running "gimp image.jpg" opens a new window in the open gimp. I think that having multiple QProcess() objects open in Scribus is not that expensive compared to the resource usage of the actual external application. Could File -> Preferences -> External Tools have a checkbox to tell Scribus that it is OK to make multiple invocations of a given tool? gimp uses D-Bus, but I am not sure if you can ask gimp to open an image through D-Bus. https://developer.gimp.org/standards.html |
|
scribus15-20160530-060532.patch (4,286 bytes)
--- scribus15/scribus/scribus.cpp- 2016-05-29 16:15:39.725407986 +0200 +++ scribus15/scribus/scribus.cpp 2016-05-30 05:55:16.272679228 +0200 @@ -299,7 +299,6 @@ resourceManager=0; UrlLauncher::instance(); m_mainWindowStatusLabel=0; - ExternalApp=0; m_ScriptRunning = 0; #ifdef Q_OS_MAC //commenting this out until this is resolved :https://bugreports.qt.io/browse/QTBUG-44565 @@ -8371,12 +8370,31 @@ slotSelect(); } -void ScribusMainWindow::imageEditorExited(int /*exitCode*/, QProcess::ExitStatus /*exitStatus*/) +void ScribusProcess::imageEditorExited(int /*exitCode*/, QProcess::ExitStatus /*exitStatus*/) { - if ( ExternalApp != 0 ) + // Mark the process inactive for purging by cleanExternalAppList + + active = false; +} + +void ScribusMainWindow::cleanExternalAppList() +{ + // Remove all of the inactive external apps + + if ( !ExternalAppList.isEmpty() ) { - delete ExternalApp; - ExternalApp = 0; + QLinkedList<ScribusProcess *>::iterator i = ExternalAppList.begin(); + while (i != ExternalAppList.end()) { + QLinkedList<ScribusProcess *>::iterator previous = i; + i++; + if ( !(*previous)->active ) + { + (*previous)->process->disconnect(); + delete (*previous)->process; + delete (*previous); + ExternalAppList.erase(previous); + } + } } } @@ -8407,17 +8425,11 @@ } #endif QString imageEditorExecutable=m_prefsManager->imageEditorExecutable(); - if (ExternalApp != 0) - { - QString ieExe = QDir::toNativeSeparators(imageEditorExecutable); - ScMessageBox::information(this, tr("Information"), "<qt>" + tr("The program %1 is already running!").arg(ieExe) + "</qt>"); - return; - } if (currItem->imageIsAvailable) { int index; QString imEditor; - ExternalApp = new QProcess(NULL); + ScribusProcess *ExternalApp = new ScribusProcess(); QStringList cmd; #if defined(_WIN32) index = imageEditorExecutable.indexOf( ".exe" ); @@ -8441,18 +8453,20 @@ if (index > -1 ) { QString imEditorDir = imEditor.left( index + 1 ); - ExternalApp->setWorkingDirectory( imEditorDir ); + ExternalApp->process->setWorkingDirectory( imEditorDir ); } cmd.append(QDir::toNativeSeparators(currItem->Pfile)); - ExternalApp->start(imEditor, cmd); - if (!ExternalApp->waitForStarted()) + ExternalApp->process->start(imEditor, cmd); + if (!ExternalApp->process->waitForStarted()) { + delete ExternalApp->process; delete ExternalApp; - ExternalApp = 0; ScMessageBox::critical(this, CommonStrings::trWarning, "<qt>" + tr("The program %1 is missing!").arg(imageEditorExecutable) + "</qt>"); return; } - connect(ExternalApp, SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(imageEditorExited(int, QProcess::ExitStatus))); + cleanExternalAppList(); + ExternalAppList.append(ExternalApp); + connect(ExternalApp->process, SIGNAL(finished(int, QProcess::ExitStatus)), ExternalApp, SLOT(imageEditorExited(int, QProcess::ExitStatus))); } } --- scribus15/scribus/scribus.h- 2016-05-16 23:19:42.119494233 +0200 +++ scribus15/scribus/scribus.h 2016-05-30 05:21:09.149240014 +0200 @@ -34,6 +34,7 @@ #include <QActionGroup> #include <QClipboard> #include <QKeyEvent> +#include <QLinkedList> #include <QMainWindow> #include <QMap> #include <QMultiHash> @@ -100,6 +101,7 @@ class ScribusCore; class ScribusDoc; class ScribusMainWindow; +class ScribusProcess; class ScribusQApp; class ScribusWin; class SimpleState; @@ -254,7 +256,8 @@ QString LoadEnc; AppModeHelper *appModeHelper; - QProcess *ExternalApp; + QLinkedList<ScribusProcess *> ExternalAppList; + void cleanExternalAppList(); QMap<QString, QPointer<ScrAction> > scrActions; QMap<QString, QPointer<ScrAction> > scrRecentFileActions; @@ -517,7 +520,6 @@ void SetSnapElements(bool b); void EditTabs(); void SearchText(); - void imageEditorExited(int, QProcess::ExitStatus); /*! \brief call gimp and wait upon completion */ void callImageEditor(); void docCheckToggle(bool visible); @@ -666,4 +668,19 @@ QQuickView *m_qqview; }; + +class ScribusProcess : public QObject +{ + Q_OBJECT + +public: + ScribusProcess() { process = new QProcess(NULL); active = true; } + + QProcess *process; + bool active; + +public slots: + void imageEditorExited(int, QProcess::ExitStatus); +}; + #endif |
|
I attached patches in scribus15-20160530-060532.patch that use a list to manage the active external apps. It allows you to open multiple images in gimp, and I think that it eventually closes the Qt connections and frees the data structures without leaving invalid references. I tested it with valgrind opening a few images, closing them, and then reopening them. William Bader, Director of Research and Development at SCS, http://www.newspapersystems.com |
|
Thanks for the patch William! |
|
on my system, scribus 1.4 can correctly open several images in gimp, 1.5 cannot. instead, of introducing even more code that controls what happens, i would prefer seeing a step back to the good old code that simply makes things work when they are properly installed and configured. i could only skim through william patch, but i have the feeling that it will behave strangely if gimp was already open when first calling it for scribus... william: can you explain what behavior your patch is introducing and why do you think scribus 1.4 could correctly talk to gimp (on my system at least) and scribus 1.5 cannot? |
|
I built scribus 1.4.7 from SVN source on my Fedora 23 laptop, and it had the same problem editing images. If I tried to edit a second image, it showed a dialog with the message "The program gimp is already running!" Scribus 1.4 has code similar to 1.5 around line 9354 of scribus/scribus.cpp to show the message if ExternalApp != 0. I don't know why scribus 1.4 works for you. If you open scribus.cpp and search for ExternalApp, once an image editor is open, the only way to reset ExternalApp to 0 is inside ScribusMainWindow::imageEditorExited(), which should not be called until the gimp command returns. In your copy of 1.4, have you removed the code in https://bugs.scribus.net/view.php?id=8753#c23215 ? Deleting the QProcess kills gimp, so scribus has to keep the QProcess around until the process signals that it is finished. If you don't want to leak resources for every process, you need to keep all of the active processes in some type of collection. I tested my patch when gimp was open before starting scribus 1.5, and there was no problem. I think that each invocation of "gimp <imagename>" returns when the window for the image is closed. Maybe gimp works differently on Windows or MacOSX. Fedora 23 has gimp-2.8.16. |
|
Thanks for the patch. Why QLinkedList? Can we generalise this any further? Eg the ability to open a raster image into Gimp and an SVG into Inkscape. Just a thought. |
|
I used QLinkedList because it has efficient deletion of items in the middle, although the list will probably remain short enough that the implementation isn't an issue. I don't do much with Qt. I can change it if there is a better collection type. Supporting different image editors is out of the scope of what I can do now. I suppose that it could work the same as some web browsers and mail clients where there is a preference setting that opens a table of mappings of file types and applications. Someone who really needed that functionality today could probably change the image handler from gimp to a script that runs "file" or something similar and then selects their prefered image editor, but that is probably not a good long term solution because the mappings would be ad hoc code in the script rather than managed by scribus. |
|
on debian testing, scribus 1.4.6 (from the repository), gimp 2.8.16 (from the repository) i get the message if gimp was not open when asking to edit the first image, but i can edit two images if gimp was already open. i'm still convinced that scribus is doing too much, and the solution is in removing something, not adding even more :-) |
|
> i get the message if gimp was not open when asking to edit the first image, but i can edit two images if gimp was already open. You can see what is happening without Scribus if you open two shell windows. Run gimp on an image in the first window. Then go to the second window and run gimp on a second image while the first gimp is still open. The second gimp will open a window and return back to the command line. In your case, if you have gimp open before starting Scribus, when you edit an image in Scribus, each gimp invocation by Scribus returns immediately, and Scribus will let you edit other images. If gimp is not open before starting Scribus, when you edit an image in Scribus, the first gimp invocation by Scribus does not return until gimp is completely closed, and Scribus will not let you edit other images. Does Scribus need to manage the process at all? Could it just run system("gimp <imagename> &") and not bother with creating a QProcess()? Does Scribus use QProcess() for portability to Windows? Was the original intent to allow only one image to be edited at once? For example, if you edit the same image twice, gimp opens a new window, so you have two open windows with the same image. Nothing is wrong with opening the same image several times at once in gimp -- I've done that intentionally to create layers and masks -- but it could be confusing for new users. Maybe the original intent was to disallow opening an image twice, but as you noticed, it is possible to get around it by opening gimp before running Scribus. |
|
Using Fedora 23/Scribus-1.5 Rev 21368 and gimp git. I am unable to edit a second image when gimp is invoked from Scribus Editing is possible if gimp is started before starting Scribus. Perhaps the manual needs to spell this out (I can't find it in the GIMP entry from DTP Toolbox) |
|
Owen, can you test the patch that I posted on May 30? |
|
Apologies, thought the patch was in the tree Using Fedora 23/Scribus-1.5 Rev 21368 and gimp git I was able to edit two images without first invoking gimp. Patch works! |
|
@owen, does it also work correctly when gimp is already open? |
|
Using Fedora 23/Scribus-1.5 Rev 21368 and gimp git. 1. Opened gimp-2.9 2. Opened scribus 3. Inserted image a Scribus new document 4. Edited that image via the menu 5. Repeated steps 3 and 4 twice, ie, a total three images 6. No problems, so can say the the patch works when gimp is already open or opened first. |
|
Thanks for testing it. When I wrote it, I did something similar, also using valgrind to check for bad memory references and some extra debug code to check that the processes were closed, but it is always better to have as many people as possible test changes. |
|
@cbradney if this doesn't interrupt dev workflow for 1.5.3, would you consider merging this ? |
|
does this patch also fix 0013524 (i can't test because i don't have photoshop) |
|
@jghali please advise |
|
ping core devs |
|
Tested Linux Mint 18; r21498; Qt5.6.1 ovencook: his method works (opened gimp - opened scribus), but... - Opened Scribus - Inserted image a Scribus new document - Image -- Edit image -> opened Gimp 2.9 - Inserted image - Image -- Edit image -> opened Gimp 2.9 The program gimp is already running! It does not work (opened scribus - opened gimp)! |
|
@william ping |
|
Peter, is that testing the original version or the version rebuilt with my patches? |
|
Original version. Source: https://launchpad.net/~scribus/+archive/ubuntu/ppa |
|
@PeterBenedek can you compile with william's patch ? |
|
Sorry Kunda, I don't know. I can't test it. :-( |
|
@jghali @cbradney I tested the patch and it works well. AFAICT with my limited understanding, FWIW... it looks safe to merge this patch. |
|
@william jghali tested your patch today. His comments on IRC: <jghali> Kunda_, i've applied it to my tree this morning, it works ok also with Photoshop, however it leaks memory <jghali> as is, the patch release the allocated QProcess only when a new image is edited <jghali> so the patch needs some modification to fix that <jghali> i've not tested that, but it also don't handle the situation where user would close scribus while image editor is still running <jghali> don't know how QProcess reacts here |
|
@william got any time to re-address the patch based on jean's feedback ? |
|
I am too busy to look at it now. I hope to look at it in a week or two. I submitted the patch six months ago, so another week or two shouldn't matter. It is not a memory leak. The link to the memory is never lost. When a process finishes, I initially tried deleting the QProcess, but Scribus crashed because it still needed something. I switched to setting a flag, and then doing the cleanup later at a safer time. The amount of memory in the QProcess structure (maybe a few KB) is significantly less than the memory used by the process (maybe 500 MB for gimp). Opening the image editor requires a few clicks, and a user would need to use the image editor about a million times without closing any images before the memory used by the data structure approached the size of even one gimp window. I could add a call to cleanExternalAppList() in the Scribus shutdown. That might be why it looked like a leak. The patches do not change what happens if you close Scribus while the image editor is open. |
|
william, the reason i ask is because the devs are close to announcing a feature freeze and I'm hoping we can get this in to trunk beforehand. |
|
I uploaded a new patch. I changed ScribusProcess::imageEditorExited() so that it deletes the QProcess immediately when an image editor process exits. I also changed ScribusMainWindow::~ScribusMainWindow() to call ScribusMainWindow::cleanExternalAppList() to handle any image editor processes that are still running when you close Scribus. I left it so that if any image editor processes are still running when you close Scribus, Scribus will leave the processes running, which leaves a QProcess around, but since Scribus is closing, it doesn't matter because the OS will reclaim the memory shortly when Scribus finishes closing. This matches the original code that did not delete ExternalApp in ~ScribusMainWindow(). If you change the second parameter to cleanExternalAppList() in ~ScribusMainWindow(), closing Scribus will delete the QProcess, which will make allocation checkers happier but can kill any open editor windows, which might lose the user's work. |
|
scribus15-20161120-072939.patch (5,103 bytes)
--- scribus15/scribus/scribus.cpp- 2016-11-19 19:23:30.103044218 +0100 +++ scribus15/scribus/scribus.cpp 2016-11-20 07:17:52.043596475 +0100 @@ -299,7 +299,6 @@ resourceManager=0; UrlLauncher::instance(); m_mainWindowStatusLabel=0; - ExternalApp=0; m_ScriptRunning = 0; #ifdef Q_OS_MAC //commenting this out until this is resolved :https://bugreports.qt.io/browse/QTBUG-44565 @@ -463,6 +462,7 @@ ScribusMainWindow::~ScribusMainWindow() { + cleanExternalAppList( /* cleanEveryApp */ true, /* killApp */ false ); // Setting killApp will kill image editor processes, which might lose the user's work if (actionManager) delete actionManager; if (appModeHelper) @@ -8383,12 +8383,46 @@ slotSelect(); } -void ScribusMainWindow::imageEditorExited(int /*exitCode*/, QProcess::ExitStatus /*exitStatus*/) +void ScribusProcess::deleteProcess(bool killProcess) { - if ( ExternalApp != 0 ) + // Disconnect and delete the QProcess. + // This will kill the image editor process if it is still running. + + if (process != NULL) { + process->disconnect(); + if (killProcess) { + delete process; + } + process = NULL; + } +} + +void ScribusProcess::imageEditorExited(int /*exitCode*/, QProcess::ExitStatus /*exitStatus*/) +{ + // Disconnect the process + + deleteProcess( /* killProcess */ true ); +} + +void ScribusMainWindow::cleanExternalAppList(bool cleanEveryApp, bool killApp) +{ + // Remove all of the inactive external apps. + // Set cleanEveryApp to true to clean all external apps associated with the window. + + if ( !ExternalAppList.isEmpty() ) { - delete ExternalApp; - ExternalApp = 0; + QLinkedList<ScribusProcess *>::iterator i = ExternalAppList.begin(); + while (i != ExternalAppList.end()) { + QLinkedList<ScribusProcess *>::iterator previous = i; + i++; + if ( cleanEveryApp && (*previous)->process != NULL ) { + (*previous)->deleteProcess(killApp); + } + if ( (*previous)->process == NULL ) { + delete (*previous); + ExternalAppList.erase(previous); + } + } } } @@ -8419,17 +8453,11 @@ } #endif QString imageEditorExecutable=m_prefsManager->imageEditorExecutable(); - if (ExternalApp != 0) - { - QString ieExe = QDir::toNativeSeparators(imageEditorExecutable); - ScMessageBox::information(this, tr("Information"), "<qt>" + tr("The program %1 is already running!").arg(ieExe) + "</qt>"); - return; - } if (currItem->imageIsAvailable) { int index; QString imEditor; - ExternalApp = new QProcess(NULL); + ScribusProcess *ExternalApp = new ScribusProcess(); QStringList cmd; #if defined(_WIN32) index = imageEditorExecutable.indexOf( ".exe" ); @@ -8453,18 +8481,20 @@ if (index > -1 ) { QString imEditorDir = imEditor.left( index + 1 ); - ExternalApp->setWorkingDirectory( imEditorDir ); + ExternalApp->process->setWorkingDirectory( imEditorDir ); } cmd.append(QDir::toNativeSeparators(currItem->Pfile)); - ExternalApp->start(imEditor, cmd); - if (!ExternalApp->waitForStarted()) + ExternalApp->process->start(imEditor, cmd); + if (!ExternalApp->process->waitForStarted()) { + delete ExternalApp->process; delete ExternalApp; - ExternalApp = 0; ScMessageBox::critical(this, CommonStrings::trWarning, "<qt>" + tr("The program %1 is missing!").arg(imageEditorExecutable) + "</qt>"); return; } - connect(ExternalApp, SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(imageEditorExited(int, QProcess::ExitStatus))); + cleanExternalAppList( /* cleanEveryApp */ false, /* killApp */ true ); + ExternalAppList.append(ExternalApp); + connect(ExternalApp->process, SIGNAL(finished(int, QProcess::ExitStatus)), ExternalApp, SLOT(imageEditorExited(int, QProcess::ExitStatus))); } } --- scribus15/scribus/scribus.h- 2016-05-16 23:19:42.119494233 +0200 +++ scribus15/scribus/scribus.h 2016-11-20 07:05:30.327014778 +0100 @@ -34,6 +34,7 @@ #include <QActionGroup> #include <QClipboard> #include <QKeyEvent> +#include <QLinkedList> #include <QMainWindow> #include <QMap> #include <QMultiHash> @@ -100,6 +101,7 @@ class ScribusCore; class ScribusDoc; class ScribusMainWindow; +class ScribusProcess; class ScribusQApp; class ScribusWin; class SimpleState; @@ -254,7 +256,8 @@ QString LoadEnc; AppModeHelper *appModeHelper; - QProcess *ExternalApp; + QLinkedList<ScribusProcess *> ExternalAppList; + void cleanExternalAppList(bool cleanEveryApp, bool killApp); QMap<QString, QPointer<ScrAction> > scrActions; QMap<QString, QPointer<ScrAction> > scrRecentFileActions; @@ -517,7 +520,6 @@ void SetSnapElements(bool b); void EditTabs(); void SearchText(); - void imageEditorExited(int, QProcess::ExitStatus); /*! \brief call gimp and wait upon completion */ void callImageEditor(); void docCheckToggle(bool visible); @@ -666,4 +668,19 @@ QQuickView *m_qqview; }; + +class ScribusProcess : public QObject +{ + Q_OBJECT + +public: + ScribusProcess() { process = new QProcess(NULL); } + void deleteProcess(bool killProcess); + + QProcess *process; + +public slots: + void imageEditorExited(int, QProcess::ExitStatus); +}; + #endif |
|
@kunda what kind of future freeze is the team going to announce? |
|
@william awesome! Thank you for the revised patch. @ale CTL is the target for 1.5.3 and is being reviewed with the intention to merge. From what's been shared on the IRC it may be occurring very soon. I'm not certain that a feature freeze will be announced outright but it makes sense given that adding code not relevant to CTL adds complexity that a feature freeze eliminates. That being said this patch could make it in to 1.5.3 with very little impact on CTL and adding a boost in useability for scribus. |
|
@william: I'm quite ok with this patch as is, however I wonder would it be possible to use QProcess::startDetached() instead and avoid the complications if Scribus is closed first? Can we still communicate with the app and send it files to edit if we detach first? |
|
I attached a new patch that uses QProcess::startDetached(). The patch is much simpler. Since startDetached() is static, there is no QProcess to connect or watch, and ExternalApp and ScribusMainWindow::imageEditorExited() are no longer needed. It seems to work OK. Scribus seems to watch the images and still detects when you save a new copy of an image. Closing Scribus does not kill any open image editor windows. Does anyone know why Scribus used start() instead of startDetached()? I ran it through valgrind, and it seems OK. The only thing is that startDetached() seems to do some fancy forking to detach the process, and valgrind warns about every allocation in Scribus at that moment as unfreed blocks in one of the short-lived forked children, but I think that those warnings can be ignored. scribus15-20161122-055029.patch (2,756 bytes)
--- scribus15/scribus/scribus.cpp- 2016-11-22 00:13:33.657620848 +0100 +++ scribus15/scribus/scribus.cpp 2016-11-22 05:25:27.269273093 +0100 @@ -299,7 +299,6 @@ resourceManager=0; UrlLauncher::instance(); m_mainWindowStatusLabel=0; - ExternalApp=0; m_ScriptRunning = 0; #ifdef Q_OS_MAC //commenting this out until this is resolved :https://bugreports.qt.io/browse/QTBUG-44565 @@ -8383,15 +8382,6 @@ slotSelect(); } -void ScribusMainWindow::imageEditorExited(int /*exitCode*/, QProcess::ExitStatus /*exitStatus*/) -{ - if ( ExternalApp != 0 ) - { - delete ExternalApp; - ExternalApp = 0; - } -} - /* call gimp and wait upon completion */ void ScribusMainWindow::callImageEditor() { @@ -8419,17 +8409,10 @@ } #endif QString imageEditorExecutable=m_prefsManager->imageEditorExecutable(); - if (ExternalApp != 0) - { - QString ieExe = QDir::toNativeSeparators(imageEditorExecutable); - ScMessageBox::information(this, tr("Information"), "<qt>" + tr("The program %1 is already running!").arg(ieExe) + "</qt>"); - return; - } if (currItem->imageIsAvailable) { int index; QString imEditor; - ExternalApp = new QProcess(NULL); QStringList cmd; #if defined(_WIN32) index = imageEditorExecutable.indexOf( ".exe" ); @@ -8450,21 +8433,13 @@ cmd.clear(); #endif index = imEditor.lastIndexOf( "/" ); - if (index > -1 ) - { - QString imEditorDir = imEditor.left( index + 1 ); - ExternalApp->setWorkingDirectory( imEditorDir ); - } + QString imEditorDir = ((index > -1)? imEditor.left( index + 1 ): "."); cmd.append(QDir::toNativeSeparators(currItem->Pfile)); - ExternalApp->start(imEditor, cmd); - if (!ExternalApp->waitForStarted()) + if (!QProcess::startDetached(imEditor, cmd, imEditorDir)) { - delete ExternalApp; - ExternalApp = 0; ScMessageBox::critical(this, CommonStrings::trWarning, "<qt>" + tr("The program %1 is missing!").arg(imageEditorExecutable) + "</qt>"); return; } - connect(ExternalApp, SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(imageEditorExited(int, QProcess::ExitStatus))); } } --- scribus15/scribus/scribus.h- 2016-05-16 23:19:42.119494233 +0200 +++ scribus15/scribus/scribus.h 2016-11-22 05:33:03.373060497 +0100 @@ -254,8 +254,6 @@ QString LoadEnc; AppModeHelper *appModeHelper; - QProcess *ExternalApp; - QMap<QString, QPointer<ScrAction> > scrActions; QMap<QString, QPointer<ScrAction> > scrRecentFileActions; QMap<QString, QPointer<ScrAction> > scrWindowsActions; @@ -517,7 +515,6 @@ void SetSnapElements(bool b); void EditTabs(); void SearchText(); - void imageEditorExited(int, QProcess::ExitStatus); /*! \brief call gimp and wait upon completion */ void callImageEditor(); void docCheckToggle(bool visible); |
|
We've been reviewing. We've found on OSX the code doesn't work as is but it leads to other requried fixes outside the cope of this patch... so some work required still. News to come. |
|
Committed with changes. If we find issues we will fix in SVN now. |
|
r21537 Thanks william, craig and jean! (also acknowledging cez and ale who worked on this issue originally) |
Date Modified | Username | Field | Change |
---|---|---|---|
2010-01-29 12:08 | cezaryece | New Issue | |
2010-01-30 11:19 | ale | Note Added: 0023152 | |
2010-01-30 11:25 | ale | Note Edited: 0023152 | |
2010-01-30 11:40 | cezaryece | Note Added: 0023153 | |
2010-01-30 12:36 | ale | Note Added: 0023154 | |
2010-01-30 12:39 | ale | Note Edited: 0023154 | |
2010-01-30 18:20 | cezaryece | Note Added: 0023164 | |
2010-01-31 10:29 | cezaryece | Note Edited: 0023164 | |
2010-01-31 17:00 | ale | Note Added: 0023179 | |
2010-02-01 08:39 | cezaryece | Note Added: 0023183 | |
2010-02-01 08:45 | cezaryece | Note Edited: 0023183 | |
2010-02-03 10:14 | ale | Note Added: 0023185 | |
2010-02-03 10:15 | ale | Note Edited: 0023185 | |
2010-02-03 10:16 | ale | Status | new => assigned |
2010-02-03 10:16 | ale | Assigned To | => ale |
2010-02-04 08:50 | cezaryece | Note Added: 0023191 | |
2010-02-04 10:53 | cezaryece | Note Added: 0023192 | |
2010-02-05 09:24 | cezaryece | Note Edited: 0023192 | |
2010-02-06 15:11 | ale | Note Added: 0023213 | |
2010-02-06 15:12 | ale | Status | assigned => confirmed |
2010-02-06 16:11 | ale | Note Added: 0023215 | |
2010-06-23 20:47 | jghali | Relationship added | has duplicate 0009203 |
2010-10-01 19:29 | jghali | Relationship added | has duplicate 0009388 |
2010-10-24 07:02 | cezaryece | Note Added: 0024706 | |
2010-10-24 07:03 | cezaryece | File Added: scribus.cpp.diff | |
2010-11-01 18:09 | Bernhard Reiter | Note Added: 0024785 | |
2010-11-01 21:13 |
|
Note Added: 0024788 | |
2010-11-01 21:14 |
|
Status | confirmed => feedback |
2010-11-02 09:57 | cezaryece | Note Added: 0024792 | |
2010-11-02 09:57 | cezaryece | Status | feedback => assigned |
2010-11-03 09:11 | ale | Note Added: 0024795 | |
2010-11-03 09:13 | ale | Note Edited: 0024795 | |
2011-11-04 15:33 | smotrs | Note Added: 0027161 | |
2013-05-07 15:58 | JLuc | Note Added: 0030132 | |
2013-05-07 15:59 | JLuc | Note Edited: 0030132 | |
2013-07-18 15:20 | JLuc | Note Edited: 0030132 | |
2016-01-26 01:21 | Kunda | Note Added: 0038433 | |
2016-01-26 18:36 | Kunda | Note Edited: 0038433 | |
2016-05-29 10:06 | Kunda | Note Added: 0041536 | |
2016-05-29 10:07 | Kunda | Tag Attached: #pending | |
2016-05-29 10:07 | Kunda | Tag Attached: Gimp | |
2016-05-29 15:20 | william | Note Added: 0041549 | |
2016-05-29 15:20 | william | Note Edited: 0041549 | |
2016-05-29 19:33 | JLuc | Tag Attached: waiting_core_dev_approval | |
2016-05-30 04:08 | william | File Added: scribus15-20160530-060532.patch | |
2016-05-30 04:20 | william | Note Added: 0041562 | |
2016-05-30 04:44 | Kunda | Patch | => Yes |
2016-05-30 04:46 | Kunda | Note Added: 0041563 | |
2016-05-30 04:46 | Kunda | Target Version | => 1.5.3 |
2016-05-31 16:01 | ale | Note Added: 0041581 | |
2016-05-31 17:05 | william | Note Added: 0041582 | |
2016-05-31 19:08 | cbradney | Note Added: 0041585 | |
2016-05-31 20:01 | william | Note Added: 0041587 | |
2016-06-02 13:43 | ale | Note Added: 0041593 | |
2016-06-02 14:22 | william | Note Added: 0041594 | |
2016-06-02 14:22 | william | Note Edited: 0041594 | |
2016-06-07 12:00 | owencook | Note Added: 0041623 | |
2016-06-07 12:43 | william | Note Added: 0041624 | |
2016-06-07 23:11 | owencook | Note Added: 0041627 | |
2016-06-08 07:07 | ale | Note Added: 0041628 | |
2016-06-08 07:32 | owencook | Note Added: 0041629 | |
2016-06-08 09:55 | william | Note Added: 0041630 | |
2016-11-01 14:57 | Kunda | Note Added: 0042282 | |
2016-11-03 21:00 | Kunda | Note Added: 0042311 | |
2016-11-03 21:01 | Kunda | Relationship added | related to 0013524 |
2016-11-07 12:02 | Kunda | Note Added: 0042354 | |
2016-11-07 23:33 | Kunda | Note View State: 0042311: public | |
2016-11-08 02:36 | Kunda | Note Added: 0042358 | |
2016-11-08 08:30 | PeterBenedek | Note Added: 0042361 | |
2016-11-08 11:45 | Kunda | Note Added: 0042362 | |
2016-11-08 15:45 | william | Note Added: 0042363 | |
2016-11-08 15:57 | PeterBenedek | Note Added: 0042364 | |
2016-11-10 18:14 | Kunda | Note Added: 0042381 | |
2016-11-10 18:34 | PeterBenedek | Note Added: 0042382 | |
2016-11-10 23:02 | Kunda | Note Added: 0042387 | |
2016-11-11 22:20 | Kunda | Note Added: 0042394 | |
2016-11-18 12:15 | Kunda | Note Added: 0042453 | |
2016-11-18 15:42 | william | Note Added: 0042460 | |
2016-11-19 03:39 | Kunda | Note Added: 0042466 | |
2016-11-20 06:50 | william | File Added: scribus15-20161120-072939.pat | |
2016-11-20 07:13 | william | Note Added: 0042477 | |
2016-11-20 07:14 | william | File Added: scribus15-20161120-072939.patch | |
2016-11-20 07:14 | william | File Deleted: scribus15-20161120-072939.pat | |
2016-11-20 11:14 | ale | Note Added: 0042478 | |
2016-11-20 11:15 | ale | Note Edited: 0042478 | |
2016-11-20 15:16 | Kunda | Note Added: 0042481 | |
2016-11-21 22:18 | cbradney | Note Added: 0042489 | |
2016-11-22 05:45 | william | File Added: scribus15-20161122-055029.patch | |
2016-11-22 05:45 | william | Note Added: 0042493 | |
2016-11-23 22:26 | cbradney | Note Added: 0042520 | |
2016-11-24 21:41 | cbradney | Assigned To | ale => cbradney |
2016-11-24 21:41 | cbradney | Status | assigned => resolved |
2016-11-24 21:41 | cbradney | Resolution | open => fixed |
2016-11-24 21:41 | cbradney | Note Added: 0042530 | |
2016-11-24 21:53 | Kunda | Note Added: 0042531 | |
2016-11-24 21:53 | Kunda | Tag Detached: waiting_core_dev_approval | |
2016-11-24 21:53 | Kunda | Tag Detached: #pending | |
2016-12-05 20:21 | cbradney | Status | resolved => closed |