Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: We should read the default config on startup #2170

Closed
wants to merge 1 commit into from
Closed

kernel: We should read the default config on startup #2170

wants to merge 1 commit into from

Conversation

cryptomilk
Copy link
Contributor

When kvirc is started it doesn't read the .kvirc4.rc config file. So every time the setup wizard pops up. The patch fixes this.

@wodim
Copy link
Member

wodim commented Jan 4, 2017

That fallback is already done here:

https://github.com/kvirc/KVIrc/blob/master/src/kvirc/kernel/KviApplication_setup.cpp#L690-L692

Called from:

https://github.com/kvirc/KVIrc/blob/master/src/kvirc/kernel/KviApplication_setup.cpp#L721

If you keep seeing the setup wizard, the problem might be somewhere else...

@Heufneutje
Copy link
Contributor

Not having that issue here. I only ever see that wizard once.

@cryptomilk
Copy link
Contributor Author

I had that with alpha2. If it has already been fixed differently a new alpha release would be nice :)

@wodim
Copy link
Member

wodim commented Jan 4, 2017

I am not sure anybody has touched that part of code. Can you try with a git version?

@cryptomilk
Copy link
Contributor Author

I've tried git master and the setup wizard opens. If I apply my patch, kvirc starts up correctly.

@cryptomilk
Copy link
Contributor Author

Here is and update to fix the KDE config. This works for me too ;)

@wodim
Copy link
Member

wodim commented Jan 4, 2017

There still are problems with this.

  1. We use Hungarian notation for our code, so please don't change the names of the variables.

  2. You have changed the config name from "kvirc" to "kvircrc". That's unnecessary and will trigger the setup wizard for all users that update.

@cryptomilk
Copy link
Contributor Author

cryptomilk commented Jan 4, 2017

  1. I don't really know what you mean exactly but I tried to change the names. I think having them consistent through the code makes sense

  2. Yes, because this is the KDE convention and this file already exists for file dialog settings

[KFileDialog Settings]
Recent Files[$e]=kvirc_337717_screenshot.png,file:$HOME/.config/KVIrc/tmp/kvirc_337717_screenshot.png
Recent URLs[$e]=file:$HOME/.config/KVIrc/tmp/

Well, all users who update will always get the setup wizard, because it is broken right now. This happened on alpha1 on alpha2 and on master. Not only on one Linux system but on several. I was just tired after several month that this setup wizard always shows up.

Starting kvirc with KF5 support always triggers the setup wizard and the KDE config file is not written so I do not see a problem with that change.

@cryptomilk
Copy link
Contributor Author

When you use KDE4 the files kvirc creates are:

$ find .kde4/ -name "kvirc*"
.kde4/share/config/kvirc
.kde4/share/config/kvircrc

The first ist the name you specify in the code to point to the kvirc configuration directory. The second is used by file dialogs. When you use kvirc built against KF5 it uses ~/.config and not ~/.kde4 until you copy the file automatically. So you get the setup wizard dialog anyway.

There is not need to have two config files around, that's why I choose kvircrc.

@ctrlaltca
Copy link
Contributor

Six years later, sorry, but this seems correct.
Please note that this only affects kvirc when built with KDE support.
Still, it will force setup to be run for users unless they

cat ~/.config/kvirc >> ~/.config/kvircrc

Adding a small piece of code to migrate the config from the old to the new file would avoid the problem. Will look into it

@ctrlaltca
Copy link
Contributor

Updated the PR at #2548, closing this one

@ctrlaltca ctrlaltca closed this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants