#40 Fixed incorrect update of KNote "last modified" field caused by the lauch of KNotes.

Merged
MicheleC merged 1 commits from issue/38/knotes into master 2 months ago

The journal last modified time was being incorrectly marked as updated when setting the color of the note. The color was set to the same values previously read from the configuration .ics file when loading a note at the launch of KNotes application.
This PR makes sure the journal is updated only is the colors are really changed, not at each call of setColor().

The journal last modified time was being incorrectly marked as updated when setting the color of the note. The color was set to the same values previously read from the configuration .ics file when loading a note at the launch of KNotes application.<br/> This PR makes sure the journal is updated only is the colors are really changed, not at each call of setColor().
MicheleC added the
PR/rfc
label 3 months ago
MicheleC added this to the R14.1.0 release milestone 3 months ago
MicheleC commented 3 months ago
Owner

Ups... looks like Kontact is still messing up things. More work will be required to fix that. Anyhow if you can, please test the KNotes related part for the time being. I will work on the Kontact part tomorrow.

Ups... looks like Kontact is still messing up things. More work will be required to fix that. Anyhow if you can, please test the KNotes related part for the time being. I will work on the Kontact part tomorrow.
MicheleC changed title from Fixed incorrect update of KNote "last modified" field caused by the lauch of KNotes. to WIP: Fixed incorrect update of KNote "last modified" field caused by the lauch of KNotes. 3 months ago
deloptes commented 2 months ago
Collaborator

Wouldn't it be better to rework knotes and kpart?

It might be more work, but at the end would be cleaner and I'm sure this problem will disappear.

I don't know if it is possible or how it would be possible, but it looks like it would require a backend to handle the notes and fromtend part - the application and another one - kpart_knotes.

It looks like to me that both are now somehow detached.

Wouldn't it be better to rework knotes and kpart? It might be more work, but at the end would be cleaner and I'm sure this problem will disappear. I don't know if it is possible or how it would be possible, but it looks like it would require a backend to handle the notes and fromtend part - the application and another one - kpart_knotes. It looks like to me that both are now somehow detached.
MicheleC commented 2 months ago
Owner

Hi Emanoil,
knotes_part is the KPart that can be embedded into other applications, like Kontact for example. KNotes is the original application, which I assume was developed before the Kpart thing.

Short term, I will work in fixing the kpart as well, I expect to be something similar to the first commit.

Long term solution would be to remove duplication of code and have KNotes to use the KPart as well, similar to what Kontact does. We can look into doing this after R14.1.0, to avoid further delay on the release of that. Also long term we need to work on a fix to get Knotes and Kontact sync together, to avoid changes in one app to be lost into the other (there is already issue #16 for that)

Hi Emanoil,<br/> knotes_part is the KPart that can be embedded into other applications, like Kontact for example. KNotes is the original application, which I assume was developed before the Kpart thing. Short term, I will work in fixing the kpart as well, I expect to be something similar to the first commit. Long term solution would be to remove duplication of code and have KNotes to use the KPart as well, similar to what Kontact does. We can look into doing this after R14.1.0, to avoid further delay on the release of that. Also long term we need to work on a fix to get Knotes and Kontact sync together, to avoid changes in one app to be lost into the other (there is already issue #16 for that)
MicheleC commented 2 months ago
Owner

PR updated with the fix for Kontact as well (just one extra line of code :smile:).
Please test and let me know if it works for you too.

PR updated with the fix for Kontact as well (just one extra line of code :smile:).<br/> Please test and let me know if it works for you too.
MicheleC changed title from WIP: Fixed incorrect update of KNote "last modified" field caused by the lauch of KNotes. to Fixed incorrect update of KNote "last modified" field caused by the lauch of KNotes. 2 months ago
deloptes commented 2 months ago
Collaborator

Hi Michele, regarding your previous post, it sounds like a very good plan.

Regarding this patch, I will try to test in the coming days, but at the end you again emit sigDataChanged whenever color was changed. I understand it is part of the note, but I am not sure it should be, however I would accept it. From first sight it looks promising.

regards

Hi Michele, regarding your previous post, it sounds like a very good plan. Regarding this patch, I will try to test in the coming days, but at the end you again emit sigDataChanged whenever color was changed. I understand it is part of the note, but I am not sure it should be, however I would accept it. From first sight it looks promising. regards
MicheleC commented 2 months ago
Owner

Hi Emanoil,
test here looks good so far, so I don't think emitting sigDataChanged is a problem. The problem was with the incorrect update of the journal entry, not the note itself.
Anyhow you may have more or different test cases from me, so you may run in different scenarios. If you testing is good as well, I will merge.

Hi Emanoil,<br/> test here looks good so far, so I don't think emitting sigDataChanged is a problem. The problem was with the incorrect update of the journal entry, not the note itself.<br/> Anyhow you may have more or different test cases from me, so you may run in different scenarios. If you testing is good as well, I will merge.
deloptes commented 2 months ago
Collaborator

Hi Michele,

yes, tests were good.

Hi Michele, yes, tests were good.
SlavekB commented 2 months ago
Owner

This looks good and at first glance it seems to me that it can also be used for r14.0.x branch.

This looks good and at first glance it seems to me that it can also be used for r14.0.x branch.
MicheleC deleted branch issue/38/knotes 2 months ago
MicheleC removed the
PR/rfc
label 2 months ago
MicheleC modified the milestone from R14.1.0 release to R14.0.8 release 2 months ago
MicheleC commented 2 months ago
Owner

Thanks Emanoil and Slavek. Merged and backported to R14.0.8 too.

Thanks Emanoil and Slavek. Merged and backported to R14.0.8 too.
The pull request has been merged as d1a4daaee6.
Sign in to join this conversation.
Loading…
Cancel
Save
There is no content yet.