Bug 316004

Summary: ktp-text-ui setting "Open new conversations:" only applies after restarting
Product: telepathy Reporter: Stefan Eggers <coloncolonone>
Component: text-uiAssignee: Telepathy Bugs <kde-telepathy-bugs>
Severity: normal CC: kde, mklapetek, Prescience500
Priority: NOR    
Version: git-latest   
Target Milestone: Future   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In: 0.6.0
Attachments: text-chat-config.h

Description Stefan Eggers 2013-03-02 11:16:59 UTC
Changing "Open new conversations:" setting in ktp-text-ui only applies after restarting ktp-text-ui. Till then it don't have any effect.

Reproducible: Always

Steps to Reproduce:
1. Open a text chat with user A from ktp-contactlist.
2. Change "Open new conversations:" to "As tabs in the same window". Exit with "OK" here.
3. Close ktp-text-ui
4. Open a text chat with user A from ktp-contactlist.
5. Change "Open new conversations:" to "As new windows". Exit with "OK" here.
6. Open a text chat with user B from ktp-contactlist.
Actual Results:  
A new tab with the chat with user B opens.

Expected Results:  
A new window with the chat with user B opens.
Comment 1 David Edmundson 2013-03-03 04:02:41 UTC
*** Bug 305159 has been marked as a duplicate of this bug. ***
Comment 2 Stefan Eggers 2013-03-03 15:44:42 UTC
Currently this happens as I understand the problem:

ktp-text-ui starts and reads the settings from file. These settings are in effect until ktp-text-ui terminates. The configuration dialog changes these settings independently in the file but don't communicate the change back anywhere to the rest of the running ktp-text-ui.

So I think a singleton shared by ktp-text-ui and it's configuration dialog should help here. It could hold the current configuration and allow to change it. Whenever ktp-text-ui needs any configurable value it will ask this singleton. Changes would take effect immediately then.

Would then also come in handy for bug 282201 (this is about allowing to turn "User is tpying" on and off) as the settings needed could be placed in the singleton, too.

I intend to give this idea a try and see if it works.
Comment 3 Martin Klapetek 2013-03-03 16:07:19 UTC
You don't necessarily need to mess with singletons, you can just emit a signal in 

void ChatWindow::showSettingsDialog()

once the dialog is destroyed, then just connect to that signal in TelepathyChatUi and reparse the configuration and apply the changes. Maybe you'll find even more elegant solution ;)
Comment 4 David Edmundson 2013-03-03 16:13:06 UTC
Then you need ChatWindow to propogate signals so the chat-ui can see them.. and then it has to know to reparse it.

I'd be ok with a singleton in this particular case.. If you want to show us a rough example of the header file of this before you proceed I'd be happy to look.
Comment 5 Martin Klapetek 2013-03-03 16:16:27 UTC
The tabOpenMode setting is parsed and stored in TelepathyChatUi, so you need to get the signal there somehow anyway; besides TelepathyChatUi creates ChatWindows and does connect to its signals already. So it would be just a matter of adding one connection there (telepathy-chat-ui.cpp:71)
Comment 6 David Edmundson 2013-03-03 16:18:40 UTC
TeleapthyChatUi needs to reload it's instance variables plus the ChatWindow needs to relay this .


(in the chat ui)
switch (Config::instance()->windowOpenMode() ) {


and in the  chat window


with all connections inside config.
Comment 7 Martin Klapetek 2013-03-03 16:26:03 UTC
Ok, since we need to actually apply the settings in multiple classes and propagating the changes would be difficult, config singleton does look like a better idea (I was thinking about changes only to the tab opening option). So all interested classes can connect to the config changes signal and update their settings.
Comment 8 Stefan Eggers 2013-03-03 17:15:06 UTC
Created attachment 77713 [details]

The singleton for the configuration settings.
Comment 9 Martin Klapetek 2013-03-03 17:17:56 UTC
Looking good! Small note - "getInstance();" -> in Qt/KDE world, we don't use the "get" keyword, so just use "instance();" ;)
Comment 10 David Edmundson 2013-03-04 11:56:50 UTC
Git commit 1b1ea8a533f72b3ccfeec554a3657ecaa9a6836e by David Edmundson, on behalf of Stefan Eggers.
Committed on 04/03/2013 at 12:55.
Pushed by davidedmundson into branch 'master'.

Combining settings in a singleton class so that there is no confusion about the current settings.

Up to now the settings for where new chats open were loaded by
ktp-text-ui on startup and not re-read anymore. Changing the settings
in the config dialog had no immediate effect; only after the next
start of ktp-text-ui it finally changed.

Now whenever the settings get asked for the singleton provides the
current value. Thus every change to the settings via the config dialog
will take effect immediately.

Also gives a way to easily add new settings in one place. Will be
useful for implementing the additional settings required by bug 282201
(make it configurable if "user is typing" gets show") which needs two
more bools.


REVIEW: 109268

M  +4    -15   app/telepathy-chat-ui.cpp
M  +0    -7    app/telepathy-chat-ui.h
M  +8    -30   config/behavior-config.cpp
M  +2    -2    config/behavior-config.h
M  +3    -0    lib/CMakeLists.txt
A  +6    -0    lib/KTp/TextChatConfig
A  +143  -0    lib/text-chat-config.cpp     [License: GPL (v2+)]
C  +28   -26   lib/text-chat-config.h [from: config/behavior-config.h - 062% similarity]     [License: GPL]