[PATCH] kopenwith.cpp bugfix and enhancement

[Willy De la Court]

Hi David,

On Tuesday 11 February 2003 13:46, David Faure wrote:
> On Monday 10 February 2003 19:48, Willy De la Court wrote:
> > ChangeLog
> > ------------------------------
> > added new checkbox "Do not &close when command exits" to the "Open
> > With..." dialog this checkbox is only visible when using konsole as set
> > in the Component chooser.
> > the checkbox is disabled until the "Run in Terminal" is activated
> >
> > added the --noclose to the TerminalOptions var but only if the
> > TerminalApplication is konsole
>
> Hmm, I'm not overly happy with one more checkbox in that dialog, for the
> sake of its usability, but it does sound like a very useful feature indeed.
>

it's good if you want to just tail a file or want to run a script that sends 
some output and does not wait.

> > fixed a bug so that Terminal is set to true when "Run in Terminal" is
> > checked
>
> ? Can you explain that one? You seem to have moved the code, but
> it does the same as before about this, doesn't it?
>
yes sure

the original code was this

  desktop.writeEntry(QString::fromLatin1("InitialPreference"), maxPreference + 
1);
  if (remember)
    if (remember->isChecked()) {
      QStringList mimeList;
      KDesktopFile oldDesktop(locate("apps", pathName), true);
      mimeList = oldDesktop.readListEntry(QString::fromLatin1("MimeType"), 
';');
      if (!qServiceType.isEmpty() && !mimeList.contains(qServiceType))
        mimeList.append(qServiceType);
      desktop.writeEntry(QString::fromLatin1("MimeType"), mimeList, ';');
      if (terminal->isChecked())
        desktop.writeEntry(QString::fromLatin1("Terminal"), true);
      else
        desktop.writeEntry(QString::fromLatin1("Terminal"), false);

This only sets the Terminal var to true if you select the remeber checkbox in 
effect if you don't want to remeber the settings it does not run the app in a 
terminal but just in the background. So i moved the if(terminal->isChecked) 
from under the condition if(remeber->ischecked()) to make it independant of 
the fact you want to remeber the setting or not.

Do you agree with my reasoning?

> > instead of adding the --noclose to the TerminalOptions I could add a new
> > option in the .desktop file but since there seems to be a block on
> > changes for .desktop files (standard) this is the best option
>
> You can add new X-KDE-Foo fields in .desktop files, that's no problem.
> But I prefer it to be in terminaloptions, since that's what it really
> is....
>

ok good

> > also this would mean changes in the krun.cpp but for the moment i want to
> > stay away from changes in something so critical
> >
> :)
> :

yes i know i looked at it and saw something about binary compatiblity and did 
not want to touch it because i'm still learning the inner workings.

> > the patch is fully functional and tested
> > can someone with write access to the cvs  commit this patch please
> > or if you don't agree with the patch please let me know
>

> The patch isn't binary compatible, since you added a member variable to
> KOpenWithDlg. 

Damn and i tought i did a good job of leaving all the functionality intact.
So at what should a look in the future to avoid these pitfalls?

> You'll need to define the Private class used by the d pointer
> and move it there.... Hmm, a better idea: remove the QPushButton* ok;
> member variable instead, it's not used.
> WOW!
> I removed it, and it shows that the while (!ok) on line 556 was using it
> instead of the bool. ARGL. This might explain a few bugs :}
> Ok I'll fix that and commit your patch as well.
> Thanks.
>
THANK YOU

You can probably expect more of those little anhancements bugfixes.
but be patient with me i'm still learning the ropes.

I look forward on the applied patch to see what i could have done better.

PS: Do you also maintain KRun? i want to discuss something with you about 
mimetype x-shellscript

Small things make people happy.
-- 
Willy De la Court