From now, when the user saves the password by clicking remember he will get a notification that the password is saved.
Details
Details
Diff Detail
Diff Detail
- Repository
- R875 Falkon
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
I am against this patch.
In my eyes I expect it to just work and to not bother me.
I would welcome notification when it for some reason failed, because that would have some value for users.
I can see first thing after we release this patch is we would get bug reports asking to disable it.
Comment Actions
sorry for this, even I thought the same but since it was discussed that it was better than nothing, I did this, if this is not good I am happy with that too.
can I close this, or do you suggest any changes?
btw thanks for confirming @SGOrava
Comment Actions
Sorry for making the previous patch a redundant one, I hope this one fixes the issue when KWallet is disabled,
when KWallet is disabled and password is tried to save falkon prompts the user to enable KWallet.
Comment Actions
The same code is repeated 5 times in different places. Wouldn't it better be factored out into a single inline function/method?
Comment Actions
@alukichev sorry, I know this is a bad mistake, will change asap and not repeat it again.
Thanks :)
Comment Actions
created a function for showing the prompt, the previous one was full of unwanted lines
Comment Actions
I actually didn't know what to use exactly and I thought that by using a message box we can also make the user click the close button in which there is a high probability that he will read. the message.
okay, I will change it to private, sorry I forgot.
If QMessage box is not suitable, I would be happy to take suggestions regarding what to use.
Comment Actions
I am not that sure about it, you see I am more of a programmer than a designer,
iirc the messagebox will block Falkon until you press it so it is a bit annoying.
Maybe using falkon notifications sounds better.
Comment Actions
Can you return original blank lines ?
The spacing between lines is there to make it easier to read.
Comment Actions
+ as drisca suggested, it would be great to add some stopper for notifications to show only once.
You may see it as "it will show only when there is a problem", well trust him with this, there will be some users who will be spammed with notifications.
I apologize I did not thought about this.
I believe it can be done simple by new bool variable which you will check and set when showing this error message.
Showing it once is enough HE said.
| src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp | ||
|---|---|---|
| 24 | keep this empty line, it separates includes from Qt and KDE | |
| 27 | no need to add so many blank lines, one is enough | |
| 88 | no need add new lines here, repeat x times | |
| 188 | no need for new line | |
| src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.h | ||
| 44 | keep this empty line | |
| 50 | keep the empty line between methods and variables | |
This comment was removed by puneethchanda.
| src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp | ||
|---|---|---|
| 24 | I am afraid, this is already there I think SGOrava, 22, emptyline(23),24 | |
Comment Actions
@SGOrava with the changes that I did now, the user gets a notification only when he clicks "remember", "update" etc right. I which case he will get only one notification.
I would really like to know what causes multiple notifications here, I would be happy to fix that. asking because of curiosity
Comment Actions
That is what you think from user perspective.
But if there is ever any bug which triggers those functions without a break a 1000 times, users will be mad.
| src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp | ||
|---|---|---|
| 24 | The empty line is not there. | |
Comment Actions
@SGOrava done. This is what you suggested right.
I have tested it and it is by calling two times, but It is not showing QDBus multiple notifications error, which I saw when I tried calling two notifications.
:)
| src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp | ||
|---|---|---|
| 176 | Probably should be called different name, as now it is no longer showing any "prompt". | |
| 179 | Don't use global static here, instead move the static inside the function. | |
| 180 | Needs to be translated (tr()) and coding style - space after comma. | |
| 183 | It's a void function so there is no reason to add empty return statement at the end. | |
Comment Actions
@drosca it's building and working for me,
and I changed the files according to your suggestions