Page MenuHomePhabricator

No strings are passed on to command line modules
Closed, ResolvedPublic

Description

If a string is supposed to be passed on from the GUI to the command line only an empty string is passed on.

Event Timeline

Hi Sascha,

please can you review CTK issue #306 and #307, which i believe fix this issue. These are not yet merged to master.

https://github.com/commontk/CTK/issues/306#issuecomment-14832070
https://github.com/commontk/CTK/issues/307#issuecomment-14832370

So, this will require a new CTK version in MITK.

Matt

zelzer added a subscriber: zelzer.Mar 14 2013, 6:22 PM

Matt, thanks a lot for working on these issues! I just did the review. Updating CTK shouldn't be a problem.

Hi Sascha,

I just updated both issue 306 and 307.

307 took much longer than I thought.... as in our project we call command line scripts that pass their parameters onto a binary, so we also had problems with spaces in our scripts, and I thought it was coming from the CLI stuff in CTK. Anyway, I think it is ok now.

So we have 2 branches:
https://github.com/commontk/CTK/commits/306-cli-string-not-passed-to-backend
and
https://github.com/commontk/CTK/commits/307-cli-empty-string-not-rejected

which I think are ok.... except I just spotted #include <iostream> unnecessarily included... anyway... I am testing it combined here:

https://github.com/NifTK/CTK/compare/306-307-niftk-test

so that I could put it in our overnight build, and get more people to take a look on Monday. But I think it should be ok.

I will let you know.

Matt

Hi Sascha,

i found a problem for this issue, where if a string parameter had a default set, and the user removed the default and intended to send an empty string, then the code automatically always replaced the empty string with a default.

THis is probably not useful.

So, I pushed this change:
https://github.com/commontk/CTK/commit/df67ad2e6912dc8d22000a9d477eb3a237df79bc

So I think that both CTK issue #306, and #307 should be OK. But I have not had time to integrate a new merged CTK (master + 306 + 307) within our NifTK build.

Matt

Hi Matt,

I think your changes are perfectly valid - thanks! I guess you could just merge them to master, checkt that the CTK unit tests still run and push everything. Maybe I can manage to update the CTK in MITK this evening.

Hi Sascha,

I just pushed to CTK master:
https://github.com/commontk/CTK/commit/5fa2949f3f984f8a2992e21001d46f3d8f7daceb

There were unit test failures, but nothing that looked relevant to this.
So I pushed anyway.

Matt

New remote branch pushed: bug-14692-update-ctk-to-fix-strings-in-cmdlinemodules

[0a9d56]: Merge branch 'bug-14692-update-ctk-to-fix-strings-in-cmdlinemodules'

Merged commits:

2013-03-26 16:26:49 Sascha Zelzer [4f7ab1]
Updated CTK to fix cmd line module string handling.

zelzer added a comment.Apr 1 2013, 4:24 PM

I think this is fixed now. Re-open this bug if there are still problems related to strings.

kislinsk edited projects, added MITK (2013-03); removed MITK.Aug 1 2016, 10:48 PM