Improved magnet link support

Agreed, I don't really see the point with magnet support if you have to paste the link to main chat or pm yourself just to use it.

--RZ

More than magnet link from terminal support, I would like to see linuxdcpp be able to register as magnet handler for the OS.

great idea. Also it can be usefull to decode escaped name and post to the chat only name and size, linked with magnet. Like Apex.

also i think good idea to do menuitem "file -> download from magnet", and then paste magnet in appeared window start download

After a conversation with zeroluck in IRC I decided I should finally finish the pipe/cmdlineparameter/thingy, that I have had lying around on my hd for a while...

I'll add a fifo to users config dir, which can be used to send commands to running linuxdcpp. Which can be used for handling magnet links.

I won't touch the actual registering with the OS, that Steven wanted (as I have no idea how to do that portably), but I'll add something like the proposed cmdline argument.

--RZ

Only thing that I ask is that you use glib (GOption) to parse command line parameters if you're not already.

For url handler registration, I don't know how to do it portably. In Gnome, we can use gconf:

gconftool-2 --set --type=string /desktop/gnome/url-handlers/magnet/command 'linuxdcpp -m "%s"'

We may look at registering it inside debian folder so at least debian-like distros can use it, but we can do that later once Razzloss adds the parameters.

OK, pushed the 1st version of this. It still needs some polishing, but feel free comment ( == bash, beat & complain the hell out of it ;p )

See linuxdcpp --help for currently implemented options. Also hub-addresses & magnets can be given as arguments without -c and -m. Attempt is made to decipher them with WulforUtil and if identified proper action is taken. It is also possible to give multiple hubs & magnets with one command this way. Currently -c and -m only support one per invocation.

--RZ

linux/WulforUtil.cc: In static member function ‘static bool WulforUtil::writeIPCCommand(std::string)’:
linux/WulforUtil.cc:387: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result

linux/mainwindow.cc: In static member function ‘static gboolean MainWindow::onExternalData(GIOChannel*, GIOCondition, void*)’:
linux/mainwindow.cc:1336: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘gsize’

Just compiled it, haven't looked at the changes. I'll look at them sometime later.

Hrmh, Either I have began to ignore warnings because of all the crap that dcpp/-spews out or I'll need to increase my compiler default warning settings (though scons should set all that up?). I'll need to check when I get home...

1st warning from wulforUtil is from a function that needs some error checking improvements anyway. So I'll get back to that with my next push.
2nd from mainwindow is from dcdebug statement, which can removed if simple casting won't fix it.

When you get to it, check out how command line arguments are handled. Do you have any objections on storing them to WulforUtil::startArgs (it's a static struct CommandLineArgs)?. I thought about putting them in WulforManager, but that didn't feel any more 'right' place than Util (plus it needs to be started). The best place would probably be MainWindow, but reaching that from main would require going thru Manager. And since the parsing is already done in Util, arguments ended up in there...

--RZ

I definitely don't like the command line argument data stored inside WulforUtil. The purpose of any util class is to provide static methods and shouldn't hold state (unless that state is static, non-changing data that needs to be used multiple times). I think we need to pass it to MainWindow after it is created in wulfor but before it enters the main loop. Another alternative is to create a new OptionsHandler class and move most of this logic to it.

--magnet and --connect need an arg description (for example, URI or <uri> for magnet)

Command line options need to be added to man page

Summary and description from man page need to be added (is there a way to keep these in sync automatically?)

I think we need to call setlocale before parsing options so the strings are converted properly to utf8

I think we need to g_option_context_free

Shouldn't we support multiple magnets and multiple hubs using only -c and -m? Or is this not possible? I don't think it makes sense to allow them to be passed without the -c/-m.

"Should we make show the default and remove the lock error message?" Yes.

Can you add N_() around the option text?

What about --existing instead of --show like xchat?

Setting up the IPC should probably be in its own method to keep the constructor from getting too big.

Did you happen to look at using dbus for IPC? It seems to be more standardized for this purpose, so we may want to look at migrating to that in the future. I don't think it will add a dependency since GTK+ itself depends on it.

Meh, I was afraid you were going to be like that ;P. Luckily it seems that commandline args can be passed directly to mainwindow after WulforManager::start and before entering the gtk main loop. (I was afraid that I have to store them in wulformanager for passing...). So this will be changed in the next commit.

arg description also in next commit. o_O we have a man page. Ok, just kidding I've noticed it before, but wouldn't have remembered it. I'll try to document them. No idea if we can synchronize them automatically with the code.

Grah, why can't glib/gtk doc. explicitly say that you have to free something. Or even a link to _free function from _new would be a plus. Anyway I'll add the free (and also freeing of the parsed args) in the next commit.

I was thinking at first to only allow one connect/magnet per command, because of the search limit of 5 seconds, but decided to add the extra argument parsing.. So yes multiple -c and -m are possible, and can be implemented, but I don't think we should remove the optionless method. Since the program 'can do the right thing', if passed well-formatted URI/magnet why shouldn't it be allowed without options?

N_() was for not translating and only mark it for text extraction? Since I believe the GLIB is supposed to translate these.

You'll need a better IRC-client :D. But anyway you're the native speaker, so if existing sounds better I don't really care.

--RZ

I think the WulforUtil::parse*Arguments() can probably be moved to main. Most other programs seem to handle command line arguments inside their main file. We can then just pass the args to mainwindow like we mentioned.

On the other hand, I've seen some programs use a combination of G_OPTION_FLAG_NO_ARG and G_OPTION_ARG_CALLBACK to connect a callback to handle the argument. Apparently it's purpose is to parse the argument, but a lot of implementations just handle it right there inside that callback. What do you think about that approach? Instead of having to store the arguments somewhere we can just handle them right there. Callbacks seem like a much cleaner approach.

There is duplicated code in MainWindow::handleCommandlineArguments_gui() and in MainWindow::onExternalData() that needs to be consolidated somehow. The previous suggestion of using a callback might take care of this since regardless of whether the arg was coming via IPC or direct, it would be handled by the same method.

Another issue is that linuxdcpp --connect --show will try to open up a hub called "--show". We need to either validate args are passed to --connect/--magnet before sending IPC or to send some delimiter in IPC command. Or both.

Yes, N_() is just so they can be extracted to the pot file for translation, so it needs to be added even if glib is calling gettext() on them. I wonder how do long name options get translated though?

Hmm...nevermind, I don't think --existing would be better. It would make sense if linuxdcpp opened up multiple instances for the same config dir and you want to tell it to not do that, but it doesn't. But if we were to make --show default like we said, is it even necessary to keep this option? I guess the reason for it in this case is that we don't want to bring the window to the foreground if a url was passed unless they explicitly say so? What do most other programs do in this scenario?

So no thoughts on dbus?

At first it was in main, but I thought it cluttered the thing a bit. I'd say a separate function is better (doesn't have to be in WulforUtil). We can move it to a regular function defined in wulfor.cc. I've already changed the parse function to use separate struct passed to instead of using one in WulforUtil. Just have to check that diff before I commit it.

I'll look into the handleCMDline & external data..

About the --connect --show, don't do it :P. I actually hoped that GOption would have done that kind of validation already, but as it seems it doesn't I don't think we should bother with the validation either. Current connect dialog already behaves similarly and does no validation at all. (Plus if you happen to add space before adc hub address in connect dialog it opens the connection, but after that just sits there...)

The current show parameter is a bit useless, since it will be changed to default action if the profile is locked. I replaced the --show with --existing, but changed the behavior so that it won't start a new instance if one isn't already running. Running linuxdpp with no parameters brings it to visible are, and giving some other command just executes the command. If other commands are combined with --existing it would bring also the window up which doesn't make sense, so maybe --show should also be added? This needs some more thinking to make the behavior logical.

Didn't really think about DBus at any point. Pipes are more lightweight and easier to handle and I think in case of multiple running instances the commands are easier to direct to specific instance (of course when linuxdcpp is used as a remote that isn't such a huge deal, but echo to pipe vs. dbus-send). Not saying that we couldn't switch to DBus at some point.

--RZ

Ok, I think most of the issues discussed are now committed.
* Manpage: Check
* Removed argument handling and storing from WulforUtils: Check
* There's now --show, which will call gtk_present on the window if combined with other options, and --existing, which will only sends commands to already running instance, if ldcpp isn't already running the other options are ignored and program exits.
* If no arguments are given and linuxdcpp is running it behaves as --show.
* Argument description added.
* Context and parsed args freed.
* N_() -added for descriptions and argument descriptions. Not for long args (and I hope those won't be translated anyway...)
* MainWindow::createIPC method added.
* Dropped the mw::handleCmdArgs... Instead added a handleArguments function to wulfor.cc which will be used incase the profile is locked and after wulformanager::start. handleArgs will write the IPC commands to pipe and ExternalData will handle them. Don't know if using arg callback would be any cleaner solution, since we can't handle the arguments while parsing (core & GUI needs to be started for -c & -m args, and I don't think we can delay parsing long enough (-V and -e needs to be handled before core startup)). So all args are first parsed to CommandlineArgs structure and handled from there.

Let me know if I missed something. And someone please test this and compare the behavior to what's documented. I'm not sure I've tested all the possibilities with the latest version.

--RZ

I added the ability to open links from web browsers and other apps by registering LinuxDC++ as the default magnet URI handler. Since I don't know how to do this portably, this change is only done for Debian based systems. I tested it and it worked in Firefox, but for some reason it did not in Epiphany.

Marking this bug as fix committed since I think all the magnet related changes are just about done. Correct me if I'm wrong Razzloss.

Is it possible to add some more commands to make linuxdcpp more scriptable.
I think of an option to immediately add a download to the download-queue. (Only possible if the magnet consists of a name, the size and the tth)
And an option to send a message to a given hub or a PM to a given user on the specified hub.

As the infrastructure is now available I think these things are just minor changes?!

Adding magnet directly to queue would useful, but I'm not sure if the dcpp-core supports adding a download without sources. Feel free to submit a separate bug/feature request about that. At least that way it will still be visible in the bug tracker after 1.1.0 release.

I actually wrote a quick 6-line patch for sending text via linuxdcpp.pipe to every connected hub for zeroluck (http://pastebin.ca/1844060). It is unlikely to end up in to the main trunk though, since it is untested, might deadlock the client and in general I think this sort of scipting would require two way communication to be useful. Something which the pipe isn't. You'd need to be able to list the connected hubs from the client and then target the specific hub. Also for success/failure status of given command you'd need the two way communication.

So no the last two aren't what I'd consider minor changes.

--RZ

I don't know what windows programmers did, but the windows clients can directly add magnets. Look for Apex, Strong and DC realisation.

I now looked at the source and found no simple/straight-forward method to add a magnet link to the queue without a "User" (Interface of dcpp is private...) and so I created a user with a random CID and removed it just afterwards.
=> http://dpaste.com/175567/
Its working for me so far. (but that means nothing) I added a fallback to search for the TTH when not all needed informations are in the link. (Saw some clients generating such links.)

You are right. Two-way would be better for chatting. Perhaps a wish depended on dbus? (I think its possible with dbus, is it?)
A socket would perhaps be a bit of overkill...

See Bug 546227 (direct magnet links) and Bug 546230 (pm).

libunique looks pretty nice. It handles all the IPC and multi-instances and seems to be heavily used.