View Issue Details

IDProjectCategoryView StatusLast Update
0008753ScribusIntegrationpublic2016-12-05 20:21
Reportercezaryece Assigned Tocbradney  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformLinuxOSKubuntuOS VersionKarmic
Product Version1.5.0svn 
Target Version1.5.3 
Summary0008753: Edit image in Gimp - must completely close Gimp to edit new image
DescriptionYou can open few images in one instance of Gimp. But not invoking it from Scribus.
Steps To Reproduce1. 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 InformationIn 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.
TagsGimp
PatchYes

Relationships

has duplicate 0009203 closedjghali Attempting to edit several photos in GIMP 
has duplicate 0009388 closedjghali Opening image editor doesn't work if editor is already open. 
related to 0013524 closed Editing Embedded Image gives a File Locked Error 

Activities

ale

2010-01-30 11:19

manager   ~0023152

Last edited: 2010-01-30 11:25

can't replicate it on debian testing... works as expected...

cezaryece

2010-01-30 11:40

updater   ~0023153

As I wrote in bug details: Kubuntu Karmic, Scribus 1.5.svn, Gimp 2.6

ale

2010-01-30 12:36

manager   ~0023154

Last edited: 2010-01-30 12:39

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"...)

cezaryece

2010-01-30 18:20

updater   ~0023164

Last edited: 2010-01-31 10:29

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.

ale

2010-01-31 17:00

manager   ~0023179

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...

cezaryece

2010-02-01 08:39

updater   ~0023183

Last edited: 2010-02-01 08:45

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!

ale

2010-02-03 10:14

manager   ~0023185

Last edited: 2010-02-03 10:15

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...

cezaryece

2010-02-04 08:50

updater   ~0023191

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?

cezaryece

2010-02-04 10:53

updater   ~0023192

Last edited: 2010-02-05 09:24

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.

ale

2010-02-06 15:11

manager   ~0023213

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.

ale

2010-02-06 16:11

manager   ~0023215

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...

cezaryece

2010-10-24 07:02

updater   ~0024706

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.

cezaryece

2010-10-24 07:03

updater  

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;
scribus.cpp.diff (878 bytes)   

Bernhard Reiter

2010-11-01 18:09

reporter   ~0024785

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).

plinnell

2010-11-01 21:13

viewer   ~0024788

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.

cezaryece

2010-11-02 09:57

updater   ~0024792

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?

ale

2010-11-03 09:11

manager   ~0024795

Last edited: 2010-11-03 09:13

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.

smotrs

2011-11-04 15:33

reporter   ~0027161

Same issue exists on Windows platform as well. Is there an equivalent scribus.cpp file to try the workaround posted above with?

JLuc

2013-05-07 15:58

developer   ~0030132

Last edited: 2013-07-18 15:20

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.

Kunda

2016-01-26 01:21

updater   ~0038433

Last edited: 2016-01-26 18:36

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!

Kunda

2016-05-29 10:06

updater   ~0041536

Jurajf, would you kindly weigh in on this issue? Thanks in advance!

william

2016-05-29 15:20

reporter   ~0041549

Last edited: 2016-05-29 15:20

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

william

2016-05-30 04:08

reporter  

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

william

2016-05-30 04:20

reporter   ~0041562

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

Kunda

2016-05-30 04:46

updater   ~0041563

Thanks for the patch William!

ale

2016-05-31 16:01

manager   ~0041581

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?

william

2016-05-31 17:05

reporter   ~0041582

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.

cbradney

2016-05-31 19:08

administrator   ~0041585

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.

william

2016-05-31 20:01

reporter   ~0041587

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.

ale

2016-06-02 13:43

manager   ~0041593

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 :-)

william

2016-06-02 14:22

reporter   ~0041594

Last edited: 2016-06-02 14:22

> 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.

owencook

2016-06-07 12:00

reporter   ~0041623

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)

william

2016-06-07 12:43

reporter   ~0041624

Owen, can you test the patch that I posted on May 30?

owencook

2016-06-07 23:11

reporter   ~0041627

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!

ale

2016-06-08 07:07

manager   ~0041628

@owen, does it also work correctly when gimp is already open?

owencook

2016-06-08 07:32

reporter   ~0041629

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.

william

2016-06-08 09:55

reporter   ~0041630

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.

Kunda

2016-11-01 14:57

updater   ~0042282

@cbradney if this doesn't interrupt dev workflow for 1.5.3, would you consider merging this ?

Kunda

2016-11-03 21:00

updater   ~0042311

does this patch also fix 0013524 (i can't test because i don't have photoshop)

Kunda

2016-11-07 12:02

updater   ~0042354

@jghali please advise

Kunda

2016-11-08 02:36

updater   ~0042358

ping core devs

PeterBenedek

2016-11-08 08:30

reporter   ~0042361

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)!

Kunda

2016-11-08 11:45

updater   ~0042362

@william ping

william

2016-11-08 15:45

reporter   ~0042363

Peter, is that testing the original version or the version rebuilt with my patches?

PeterBenedek

2016-11-08 15:57

reporter   ~0042364

Original version.

Source: https://launchpad.net/~scribus/+archive/ubuntu/ppa

Kunda

2016-11-10 18:14

updater   ~0042381

@PeterBenedek can you compile with william's patch ?

PeterBenedek

2016-11-10 18:34

reporter   ~0042382

Sorry Kunda, I don't know. I can't test it. :-(

Kunda

2016-11-10 23:02

updater   ~0042387

@jghali @cbradney I tested the patch and it works well. AFAICT with my limited understanding, FWIW... it looks safe to merge this patch.

Kunda

2016-11-11 22:20

updater   ~0042394

@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

Kunda

2016-11-18 12:15

updater   ~0042453

@william got any time to re-address the patch based on jean's feedback ?

william

2016-11-18 15:42

reporter   ~0042460

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.

Kunda

2016-11-19 03:39

updater   ~0042466

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.

william

2016-11-20 07:13

reporter   ~0042477

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.

william

2016-11-20 07:14

reporter  

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

ale

2016-11-20 11:14

manager   ~0042478

Last edited: 2016-11-20 11:15

@kunda what kind of future freeze is the team going to announce?

Kunda

2016-11-20 15:16

updater   ~0042481

@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.

cbradney

2016-11-21 22:18

administrator   ~0042489

@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?

william

2016-11-22 05:45

reporter   ~0042493

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);

cbradney

2016-11-23 22:26

administrator   ~0042520

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.

cbradney

2016-11-24 21:41

administrator   ~0042530

Committed with changes. If we find issues we will fix in SVN now.

Kunda

2016-11-24 21:53

updater   ~0042531

r21537
Thanks william, craig and jean!
(also acknowledging cez and ale who worked on this issue originally)

Issue History

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 plinnell Note Added: 0024788
2010-11-01 21:14 plinnell 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