Opened 12 years ago

Closed 12 years ago

#1559 closed Enhancement (fixed)

Simplify tr_sessionInitFull

Reported by: KyleK Owned by: charles
Priority: Normal Milestone: 1.50
Component: Transmission Version: 1.40
Severity: Normal Keywords:
Cc:

Description

The function tr_sessionInitFull in its current form has a staggering 31 arguments. Adding new options (e.g. on the commandline, or settings file) means adding new arguments to this function.

The proposed patch unclutters the function itself by moving all settings to a separate object. This should make it a lot easier to add functionality.

It is also possible to create version-specific preferences. If the GTK version wants to add a new option, only the struct has to be extended.

I made this patch because I intend to make some preferences available in the GTK version also available for the daemon later on. I really don't want to add more arguments to the function.

I've so far only adapted the daemon to the new version, I'll update the CLI if you accept this patch.

The patch also removes the function tr_sessionInit(), which doesn't seem to be used anymore. That's just personal preference though.

Let me know what you think.

Attachments (4)

session_init.patch (20.7 KB) - added by KyleK 12 years ago.
session_init2.patch (29.2 KB) - added by KyleK 12 years ago.
session_init3.patch (23.9 KB) - added by KyleK 12 years ago.
settings.diff (126.6 KB) - added by charles 12 years ago.
development patch that integrates with daemon, cli, gtk

Download all attachments as: .zip

Change History (17)

Changed 12 years ago by KyleK

comment:1 Changed 12 years ago by charles

  • Milestone changed from None Set to 1.50
  • Owner set to charles
  • Status changed from new to assigned

What do I think? I think this is extremely close to what I'd had in mind for doing to this function before 1.50, so, good work. ;)

One difference, though, is that instead of having a custom structure for this, I'd like to pass in a tr_benc* that holds a dictionary of key/value pairs for all these fields. That way we could essentially pass in a settings.json directly to tr_sessionInit(). What do you think of that idea?

In order to keep the daemon, cli, and gtk apps from each reinventing the wheel on startup and shutdown, there probably also needs to be a tr_sessionGetDefaultSettings() and tr_sessionGetSettings().

comment:2 Changed 12 years ago by KyleK

Nice, and I thought the daemon was the step child of the bunch :)

I agree that a tr_benc structure is an even better choice. Moving all of the dealings with settings (reading and writing) and TM initialization to the library will make it much much easier to add new options and preferences.

comment:3 Changed 12 years ago by charles

KyleK: do you want to modify your patch for that? :)

comment:4 Changed 12 years ago by KyleK

Already on it :)

comment:5 Changed 12 years ago by charles

There also needs to be a way to merge together an existing benc-or-settings.json with the default settings, so that when we add new settings the default values will be picked up, but values from the benc-or-settings.json will override the defaults.

I'm not sure how this should be handled. Maybe there should be a new function that takes two benc pointers and has one overwrite the values in the other. If you have a better idea feel free to use it instead.

comment:6 Changed 12 years ago by charles

KyleK, is there an ETA for this?

comment:7 Changed 12 years ago by KyleK

I'm currently targetting the weekend, but I might have some free time tomorrow.

comment:8 Changed 12 years ago by KyleK

Ok, first steps are done.

The attached patch performs numerous changes to the way a session is initialized. tr_sessionInitFull() now has only a single tr_benc* structure as argument.

There are 2 new functions:

  • tr_getDefaultSettings() creates a tr_benc* struct with a couple of default preferences.
  • tr_bencMergeDicts() merges the content of 2 tr_benc* dictionaries into one. Different values are overwritten, key:value pairs not present in the first structure are added.

The daemon has been updated to use the new methods. Creating settings is now done in 3 steps:

  1. create a tr_benc object with default settings (tr_getDefaultSettings())
  1. Merge this object with a tr_benc object retrieved from settings.json
  1. Overwrite certain values if the user specified them on the command-line

Then pass the final tr_benc to tr_sessionInitFull() and all should be well.

So far I've not had the time to test all this, so the worst should be expected :) I've added an additional test to bencode-test.c to test the merging of 2 tr_benc's At least the test I've written produces no failures :)

Let me know what you think.

Changed 12 years ago by KyleK

comment:9 Changed 12 years ago by charles

This patch doesn't apply cleanly to svn trunk... daemon.c and session.c both fail. :(

comment:10 Changed 12 years ago by charles

the bencode.c and bencode-test.c changes are checked into r7359 now.

It'll probably be easier to copy the new bencode.[ch] files into your workarea, since I also took the opportunity to clean up the header indentations and replace ints with tr_bools where appropriate.

comment:11 Changed 12 years ago by KyleK

This new patch applies cleanly to the current svn version (r7361).

Good rewrite of tr_MergeDicts btw. I had been looking for a method to determine the size of a dict, but it wasn't there yet :)

Changed 12 years ago by KyleK

Changed 12 years ago by charles

development patch that integrates with daemon, cli, gtk

comment:12 Changed 12 years ago by charles

big commit for libT, gtk, daemon, cli in r7367

comment:13 Changed 12 years ago by livings124

  • Resolution set to fixed
  • Status changed from assigned to closed

r7370 for mac

Note: See TracTickets for help on using tickets.