#3 Proposal to extend the dcop interface for knotes

Open
deloptes wants to merge 9 commits from feat/knotes_interface_extention into master
deloptes commented 1 year ago

Bug 2691 - Proposal to extend the dcop interface for knotes.

I want to herewith propose extending the dcop knotes interface with a getRevision() method. This would be of great advantage for synchronization. I am using this for synchronization since 2016-09-20. IT works just great, while before there was no way to get the last modified of the item, although it is stored and updated accordingly in the underlaying object.

All details https://bugs.trinitydesktop.org/show_bug.cgi?id=2691

Bug 2691 - Proposal to extend the dcop interface for knotes. I want to herewith propose extending the dcop knotes interface with a getRevision() method. This would be of great advantage for synchronization. I am using this for synchronization since 2016-09-20. IT works just great, while before there was no way to get the last modified of the item, although it is stored and updated accordingly in the underlaying object. All details https://bugs.trinitydesktop.org/show_bug.cgi?id=2691
MicheleC commented 1 year ago
Owner

Hi Emanoil,

I started looking into this. I noticed that the patch in bugszilla includes changes in 3 more files than this PR. Are the changes in those files not required? Or you just mistakenly forgot them?

tdepim/kontact/plugins/knotes/knotes_part.cpp
tdepim/kontact/plugins/knotes/knotes_part.h
tdepim/kontact/plugins/knotes/knotes_plugin.cpp

Please advice.

Hi Emanoil, I started looking into this. I noticed that the patch in bugszilla includes changes in 3 more files than this PR. Are the changes in those files not required? Or you just mistakenly forgot them? tdepim/kontact/plugins/knotes/knotes_part.cpp<br> tdepim/kontact/plugins/knotes/knotes_part.h<br> tdepim/kontact/plugins/knotes/knotes_plugin.cpp<br> Please advice.
deloptes commented 1 year ago
Poster

Hi, perhaps it is the error of the newbie, but I think I downloaded the patch and applied it to the fresh repository. Why it was missing I have no idea.

I did committed the missing changes. If you are so kind to look at them. These are needed for the konqueror part to work properly.

kind regards

Hi, perhaps it is the error of the newbie, but I think I downloaded the patch and applied it to the fresh repository. Why it was missing I have no idea. I did committed the missing changes. If you are so kind to look at them. These are needed for the konqueror part to work properly. kind regards
MicheleC commented 1 year ago
Owner

Thanks, I will look at merging this over the weekend.

Thanks, I will look at merging this over the weekend.
MicheleC commented 1 year ago
Owner

Hi Emanoil,

I looked at the code. There are a few points I am not quite clear about, so IMO more work is required on this patch before we can merge it. Please correct me if my understanding is wrong.

  1. KNote::getLastModified() This function seems unnecessary to me. It exports the last modified date/time and it is used only to call KNote::setJournalLastModified() to set again the last modified time/date of the same note.
    It seems to me that the line at knotes/knote.cpp:431 in your patch would achieve the same result without all the other calls to get or set the modified date/time, after you modify it to call the similar function on the journal object.

  2. In KNote::saveData(), I don't understand why the m_lastModified is not saved here but only later when saving the journal.

  3. In KNotesApp::saveNotes() you are adjusting the last modified time/date before actually saving the notes.
    A. First, I find this not correct, since the last modified time/date of a note should be set when calling KNote::save()
    B. Second, your implementation requires a double loop through all notes, first to set the time/date and then to actually save them (inside the m_manager->save() function, which in turn calls the KNote::save()
    C. Third, as a consequence of A. and B., the additional functions in KNotesApp and KNotesPart seem uncessary too.

  4. some changes are only spaces/tabs and could be avoided to make the patch smaller to look at.

  5. would be better to remove unnecessary comments before submitting the patch.

I am not very sure how to test the functionality of the patch. If you can provide a simple dumb-proof sequence of steps, I will be happy to try that out.

If I am misunderstanding part of the code, please correct me and point me in the right way. Otherwise please rework the patch and when you think it is again ready, reset the PR/not-ok label, so that I will check again.

Hi Emanoil, I looked at the code. There are a few points I am not quite clear about, so IMO more work is required on this patch before we can merge it. Please correct me if my understanding is wrong. 1) KNote::getLastModified() This function seems unnecessary to me. It exports the last modified date/time and it is used only to call KNote::setJournalLastModified() to set again the last modified time/date of the same note.<br> It seems to me that the line at knotes/knote.cpp:431 in your patch would achieve the same result without all the other calls to get or set the modified date/time, after you modify it to call the similar function on the journal object. 2) In KNote::saveData(), I don't understand why the m_lastModified is not saved here but only later when saving the journal. 3) In KNotesApp::saveNotes() you are adjusting the last modified time/date before actually saving the notes.<br> A. First, I find this not correct, since the last modified time/date of a note should be set when calling KNote::save()<br> B. Second, your implementation requires a double loop through all notes, first to set the time/date and then to actually save them (inside the m_manager->save() function, which in turn calls the KNote::save()<br> C. Third, as a consequence of A. and B., the additional functions in KNotesApp and KNotesPart seem uncessary too. 4) some changes are only spaces/tabs and could be avoided to make the patch smaller to look at. 5) would be better to remove unnecessary comments before submitting the patch. I am not very sure how to test the functionality of the patch. If you can provide a simple dumb-proof sequence of steps, I will be happy to try that out. If I am misunderstanding part of the code, please correct me and point me in the right way. Otherwise please rework the patch and when you think it is again ready, reset the PR/not-ok label, so that I will check again.
MicheleC added the
PR/not-ok
label 1 year ago
deloptes commented 1 year ago
Poster

Hi Michele, thank you for looking into the patch. Your comments are fair enough. The problem is it was 2y ago and I have no memories why changes are like they are. I have to roll out again an environment for testing, which will take time. I recall it all was tricky, but as I gained more knowledge over those two years, I think it is worth looking again into that and learning even more about TDE. I will look forward to catch up with that as priorities allow and will comment on the points raised, please be patient. Regarding testing - my use case was synchronization with syncevolution over bluetooth and my test cases were based on that (create note on phone, sync with TDE, modify note in TDE and sync back, also in the other direction). I don't know if you can repeat those steps - perhaps a simple file based synchronization can be used. I'll keep this in mind when dealing with the points and I hope it is OK for you.

Hi Michele, thank you for looking into the patch. Your comments are fair enough. The problem is it was 2y ago and I have no memories why changes are like they are. I have to roll out again an environment for testing, which will take time. I recall it all was tricky, but as I gained more knowledge over those two years, I think it is worth looking again into that and learning even more about TDE. I will look forward to catch up with that as priorities allow and will comment on the points raised, please be patient. Regarding testing - my use case was synchronization with syncevolution over bluetooth and my test cases were based on that (create note on phone, sync with TDE, modify note in TDE and sync back, also in the other direction). I don't know if you can repeat those steps - perhaps a simple file based synchronization can be used. I'll keep this in mind when dealing with the points and I hope it is OK for you.
MicheleC commented 1 year ago
Owner

Hi Emanoil,

no problem, take your time. R14.1.0 won't be release soon, so there is no rush.

Re testing, I don't think I can reproduce the phone-TDE sync thing, so if you find a way to test using normal files (create by hand, sync, modify on TDE, check result by hand) that would be great :+1:

Hi Emanoil, no problem, take your time. R14.1.0 won't be release soon, so there is no rush. Re testing, I don't think I can reproduce the phone-TDE sync thing, so if you find a way to test using normal files (create by hand, sync, modify on TDE, check result by hand) that would be great :+1:
deloptes commented 1 year ago
Poster

I did revisit knote. I will work on the rest in the next couple of days.

I did revisit knote. I will work on the rest in the next couple of days.
deloptes commented 1 year ago
Poster

Hi, unfortunately I can not build tdepim now because of

/mnt/DEVELOPMENT/TDE/repo-test/tde/2_build/tdepim/obj-x86_64-linux-gnu/ktnef/gui/attachpropertydialogbase.cpp:100:56: error: invalid use of incomplete type 'class TDEListView'
     properties_ = new TDEListView( this, "properties_" );

Do you have a hint on how to proceed?

Thanks

Hi, unfortunately I can not build tdepim now because of ``` /mnt/DEVELOPMENT/TDE/repo-test/tde/2_build/tdepim/obj-x86_64-linux-gnu/ktnef/gui/attachpropertydialogbase.cpp:100:56: error: invalid use of incomplete type 'class TDEListView' properties_ = new TDEListView( this, "properties_" ); ``` Do you have a hint on how to proceed? Thanks
SlavekB commented 1 year ago
Owner

You have the current version of tdepim – newer than commit 0d192ed6?

You have the current version of tdepim – newer than commit 0d192ed6?
deloptes commented 1 year ago
Poster

I do not think so, as I did this to the branch feat/knotes_interface_extention.

What is the right way to proceed?

Should I stash or remove this branch and start a new one?

You know I am not a git guru, so thank you in advance.

I do not think so, as I did this to the branch feat/knotes_interface_extention. What is the right way to proceed? Should I stash or remove this branch and start a new one? You know I am not a git guru, so thank you in advance.
SlavekB commented 1 year ago
Owner

Your branch was based on a seven month old base. I did a rebase to the current head. On your side you use:

git pull --rebase

This should update your local branch by branch on server => will be based on the current code.

Your branch was based on a seven month old base. I did a rebase to the current head. On your side you use: ``` git pull --rebase ``` This should update your local branch by branch on server => will be based on the current code.
deloptes commented 1 year ago
Poster

Hi Slavek, thank you, however I still have problem compiling pieces unrelated to this patch

/mnt/DEVELOPMENT/TDE/repo-test/tde/2_build/tdepim/tderesources/caldav/resource.cpp:380:223: error: no matching function for call to 'KPasswordDialog::getPassword(TQString&, const TQString, NULL)'
 i18n("Remote authorization required") + TQString("</b><p>") + i18n("Please input the password for") + TQString(" ") + mPrefs->getusername(), NULL) != 1) {
                                                                                                                                                  ^
In file included from /mnt/DEVELOPMENT/TDE/repo-test/tde/2_build/tdepim/tderesources/caldav/resource.cpp:29:0:
/opt/trinity/include/kpassdlg.h:359:16: note: candidate: static int KPasswordDialog::getPassword(TQCString&, TQString, int*)
     static int getPassword(TQCString &password, TQString prompt, int *keep=0L);
                ^~~~~~~~~~~

Can you help me out please?

I think I have to update all and build dependencies - I will try this, but not quite sure if it would help.

Thank in advance

regards

Hi Slavek, thank you, however I still have problem compiling pieces unrelated to this patch ``` /mnt/DEVELOPMENT/TDE/repo-test/tde/2_build/tdepim/tderesources/caldav/resource.cpp:380:223: error: no matching function for call to 'KPasswordDialog::getPassword(TQString&, const TQString, NULL)' i18n("Remote authorization required") + TQString("</b><p>") + i18n("Please input the password for") + TQString(" ") + mPrefs->getusername(), NULL) != 1) { ^ In file included from /mnt/DEVELOPMENT/TDE/repo-test/tde/2_build/tdepim/tderesources/caldav/resource.cpp:29:0: /opt/trinity/include/kpassdlg.h:359:16: note: candidate: static int KPasswordDialog::getPassword(TQCString&, TQString, int*) static int getPassword(TQCString &password, TQString prompt, int *keep=0L); ^~~~~~~~~~~ ``` Can you help me out please? I think I have to update all and build dependencies - I will try this, but not quite sure if it would help. Thank in advance regards
MicheleC commented 1 year ago
Owner

Hi Emanoil,
I suggest you perform a full update from gitea, including tde-packaging. There has been a lot of code changing in the last few months, especially on master, so depending on how old is your local copy, you may have FTBFS.

If it still FTBFS after a full update, then there is a bug and we will need to look into it.

Hi Emanoil, <br> I suggest you perform a full update from gitea, including tde-packaging. There has been a lot of code changing in the last few months, especially on master, so depending on how old is your local copy, you may have FTBFS. If it still FTBFS after a full update, then there is a bug and we will need to look into it.
MicheleC commented 1 year ago
Owner

Just for info, I just finished rebuilding tdepim with this branch in debian buster and it was successful.

Just for info, I just finished rebuilding tdepim with this branch in debian buster and it was successful.
deloptes commented 1 year ago
Poster

I just tried following, updating all incl. tde-packages in master, patching tdepim and trying to build. It failed again on the password as mentioned above. I'll investigate, if I have missed something.

Are you also at master? You have been working on password and might be something is still not in master.

I'll report later.

Thanks im advance

regards

I just tried following, updating all incl. tde-packages in master, patching tdepim and trying to build. It failed again on the password as mentioned above. I'll investigate, if I have missed something. Are you also at master? You have been working on password and might be something is still not in master. I'll report later. Thanks im advance regards
MicheleC commented 1 year ago
Owner

Hi Emanoil, all my sources are on master, including tde-packaging. The only exception was tdepim, which I built on this branch for testing ;-)

Hi Emanoil, all my sources are on master, including tde-packaging. The only exception was tdepim, which I built on this branch for testing ;-)
deloptes commented 1 year ago
Poster

I found the problem with the test repo where I build.

I will report back after I test the tdepim package and make sure it works, but can you have a look at the patch now.

regards

I found the problem with the test repo where I build. I will report back after I test the tdepim package and make sure it works, but can you have a look at the patch now. regards
MicheleC commented 1 year ago
Owner

Hi Emanoil,
thanks for the updated patch. I would like to be able to somehow test locally with and without the patch, to have a better understanding of the original problem and what your fix does. I don't have an old phone to sync to, so if you are able to provide a procedure to test in different ways (maybe create test file, do something, check result or similar) it would be great.

At first sight, the new version does not seem much different from the old one, that is why I would like to test first. I don't like providing wrong feedback based only on assumption :wink:

Hi Emanoil,<br> thanks for the updated patch. I would like to be able to somehow test locally with and without the patch, to have a better understanding of the original problem and what your fix does. I don't have an old phone to sync to, so if you are able to provide a procedure to test in different ways (maybe create test file, do something, check result or similar) it would be great. At first sight, the new version does not seem much different from the old one, that is why I would like to test first. I don't like providing wrong feedback based only on assumption :wink:
deloptes commented 1 year ago
Poster

Hi, I was looking into options for testing and I do not see a way that it can be easily done, because the notes seem to be somehow a special case and no application (sync service) is providing a memo/notes that can be used for simulation. All I have seen is contacts, calendar and todo. So the task is challenging one.

I will think of it further. However I cleaned up some part of the code that was indeed not needed and tested the sync with new build code which is working fine. The rest is self explaining I think. But if needed I can explain in detail.

Hi, I was looking into options for testing and I do not see a way that it can be easily done, because the notes seem to be somehow a special case and no application (sync service) is providing a memo/notes that can be used for simulation. All I have seen is contacts, calendar and todo. So the task is challenging one. I will think of it further. However I cleaned up some part of the code that was indeed not needed and tested the sync with new build code which is working fine. The rest is self explaining I think. But if needed I can explain in detail.
MicheleC commented 1 year ago
Owner

ok, thanks. I will take a look in the next days and comment back. I will also think if there is a way to test locally :wink:

ok, thanks. I will take a look in the next days and comment back. I will also think if there is a way to test locally :wink:
deloptes commented 1 year ago
Poster

I am not sure what you want to test exactly, but creating a note with the patch results in timestamp being saved in the journal under LAST-MODIFIED.

This helps work out the sync direction very accurate opposed to the text comparing used in opensync (I speak from memory) for example. This is also the same logic used in the other sync modules in syncevolution, which is using very strict implementation of the vcal and vcard standards.

The problem solved was the lack of this property in the interface, making it very hard to catch the changes and accurately determine the direction of the sync.

As tde/knotes is using the vcal journal format in the background, it was intuitive and cheep to provide this property to the interface

  • Example
BEGIN:VJOURNAL
DTSTAMP:20181225T215359
X-TDE-KNotes-BgColor:#ffff00
X-TDE-KNotes-FgColor:#000000
X-TDE-KNotes-RichText:false
CREATED:20181225T215348
UID:libkcal-1057209850.794
LAST-MODIFIED:20181225T215358
DESCRIPTION:Test note
SUMMARY:25.12.2018 21:53
END:VJOURNAL

I then sync, modify on the phone and sync again

BEGIN:VJOURNAL
DTSTAMP:20181225T220156
X-TDE-KNotes-BgColor:#ffff00
X-TDE-KNotes-FgColor:#000000
X-TDE-KNotes-RichText:false
CREATED:20181225T215348
UID:libkcal-1057209850.794
LAST-MODIFIED:20181225T220156
DESCRIPTION:Test note\n\n mod1
SUMMARY:25.12.2018 21:53
END:VJOURNAL

I am sure you can try on your end without the patch and see the difference.

I am not sure what you want to test exactly, but creating a note with the patch results in timestamp being saved in the journal under LAST-MODIFIED. This helps work out the sync direction very accurate opposed to the text comparing used in opensync (I speak from memory) for example. This is also the same logic used in the other sync modules in syncevolution, which is using very strict implementation of the vcal and vcard standards. The problem solved was the lack of this property in the interface, making it very hard to catch the changes and accurately determine the direction of the sync. As tde/knotes is using the vcal journal format in the background, it was intuitive and cheep to provide this property to the interface * Example ``` BEGIN:VJOURNAL DTSTAMP:20181225T215359 X-TDE-KNotes-BgColor:#ffff00 X-TDE-KNotes-FgColor:#000000 X-TDE-KNotes-RichText:false CREATED:20181225T215348 UID:libkcal-1057209850.794 LAST-MODIFIED:20181225T215358 DESCRIPTION:Test note SUMMARY:25.12.2018 21:53 END:VJOURNAL ``` I then sync, modify on the phone and sync again ``` BEGIN:VJOURNAL DTSTAMP:20181225T220156 X-TDE-KNotes-BgColor:#ffff00 X-TDE-KNotes-FgColor:#000000 X-TDE-KNotes-RichText:false CREATED:20181225T215348 UID:libkcal-1057209850.794 LAST-MODIFIED:20181225T220156 DESCRIPTION:Test note\n\n mod1 SUMMARY:25.12.2018 21:53 END:VJOURNAL ``` I am sure you can try on your end without the patch and see the difference.
MicheleC commented 1 year ago
Owner

Great, this is a good suggestion. I will look into it after working on the tdewallet problem.

Great, this is a good suggestion. I will look into it after working on the tdewallet problem.
MicheleC commented 1 year ago
Owner

Hi Emanoil, one more question. When you sync, you are using the sync button in top right corner of Kontact, right? Or are you syncing from KDCOP?

Hi Emanoil, one more question. When you sync, you are using the sync button in top right corner of Kontact, right? Or are you syncing from KDCOP?
deloptes commented 1 year ago
Poster

Hi Michele, I must disappoint you. I did not understand what is the sync button on the menu. I once had a look at it and it pointed to cal/carddav. When I refer to sync I mean sync via syncevolution with syncml capable phone (over bluetooth).

So I have configured syncevolution for my N9 and run a script that invokes syncevolution with the configuration for the phone.

Hi Michele, I must disappoint you. I did not understand what is the sync button on the menu. I once had a look at it and it pointed to cal/carddav. When I refer to sync I mean sync via syncevolution with syncml capable phone (over bluetooth). So I have configured syncevolution for my N9 and run a script that invokes syncevolution with the configuration for the phone.
MicheleC commented 1 year ago
Owner

Hi Emanoil,
I have been doing some testing and looking at this in more details. First of all, my apologies: when I first look at the code I missed some of the context and so some of my notes in this comment are not correct.

Adding the method “getLastModified” to DCOP interface is fine. Nevertheless the use of the member “m_lastmodified” seems unnecessary to me. We could just use the journal for that. See futher comments in the code review I will do after this post.

I have also found several issues with knotes and kontacts, which affect the time in “last modified” field. Perhaps this was at the root of the problem you originally reported on bugzilla. See #15 and #16 for additional info.

Hi Emanoil,<br> I have been doing some testing and looking at this in more details. First of all, my apologies: when I first look at the code I missed some of the context and so some of my notes in this [comment](https://mirror.git.trinitydesktop.org/gitea/TDE/tdepim/pulls/3#issuecomment-643) are not correct. Adding the method "getLastModified" to DCOP interface is fine. Nevertheless the use of the member "m_lastmodified" seems unnecessary to me. We could just use the journal for that. See futher comments in the code review I will do after this post. I have also found several issues with knotes and kontacts, which affect the time in "last modified" field. Perhaps this was at the root of the problem you originally reported on bugzilla. See #15 and #16 for additional info.
MicheleC reviewed 1 year ago
I have not yet review the knotes_part of the PR
knotes/knote.cpp
@@ -153,6 +153,7 @@ KNote::KNote( TQDomDocument buildDoc, Journal *j, TQWidget *parent, const char *
m_label->setLineWidth( 0 );
m_label->installEventFilter( this ); // receive events (for dragging & action menu)
setName( m_journal->summary() ); // don't worry, no signals are connected at this stage yet
m_lastmodified = m_journal->lastModified();
MicheleC

The use of “m_lastmodified” can be avoided in my opinion by using the journal. see comment further below.

The use of "m_lastmodified" can be avoided in my opinion by using the journal. see comment further below.
knotes/knote.cpp
@@ -426,2 +428,4 @@
m_journal->setSummary( m_label->text() );
m_journal->setDescription( m_editor->text() );

m_lastmodified = TQDateTime::currentDateTime();
MicheleC

Also it would be wrong to modify the time of “last modified” each time a note is saved.

Also it would be wrong to modify the time of "last modified" each time a note is saved.
knotes/knote.cpp
@@ -454,2 +461,4 @@
}

TQDateTime KNote::getLastModified() const
{
MicheleC

Why not returning m_journal->lastModified value of this note? Or is that value specific for the journal and not the note? From what I see while testing with knotes, the value of the last modified field is correctly updated each time a note is modified, while for unmodified notes, the value is not changes when saving the .ics file

Why not returning m_journal->lastModified value of this note? Or is that value specific for the journal and not the note? From what I see while testing with knotes, the value of the last modified field is correctly updated each time a note is modified, while for unmodified notes, the value is not changes when saving the .ics file
knotes/knote.cpp
@@ -456,0 +465,4 @@
return m_lastmodified;
}

void KNote::setLastModified(const TQDateTime &dt)
MicheleC

This function is not use anywhere and not part of the DCOP interface. It could be safely removed.

This function is not use anywhere and not part of the DCOP interface. It could be safely removed.
knotes/knotesapp.cpp
@@ -345,0 +348,4 @@
TQDateTime d;
if ( note )
d = note->getLastModified();
if (!d.isValid())
MicheleC

This function should return the value of the last modified field for the selected note. Checking whether the value is valid of not, should not be done here. Either it should be done before saving a note or otherwise in the application that is actually calling this function. It would be a cleaner design.
Not withstanding that a note should never be saved with an incorrect time in first place.

This function should return the value of the last modified field for the selected note. Checking whether the value is valid of not, should not be done here. Either it should be done before saving a note or otherwise in the application that is actually calling this function. It would be a cleaner design.<br> Not withstanding that a note should never be saved with an incorrect time in first place.
knotes/resourcemanager.cpp
@@ -106,3 +108,4 @@
// TODO: only emit the signal if the journal is new?
m_resourceMap.insert( journal->uid(), resource );
emit sigRegisteredNote( journal );
journal->setLastModified(dt);
MicheleC

This seems totally unnecessary here. The note should already come here with the correct time.

This seems totally unnecessary here. The note should already come here with the correct time.
MicheleC commented 1 year ago
Owner

This is the first part of the review, I have not yet looked at the knotes_part section. I will do that after you have worked on my previous comments.

Let me know if anything is unclear or if you think some of my comments are wrong.

This is the first part of the review, I have not yet looked at the knotes_part section. I will do that after you have worked on my previous comments. Let me know if anything is unclear or if you think some of my comments are wrong.
MicheleC added the
PR/wip
label 1 year ago
MicheleC removed the
PR/not-ok
label 1 year ago
deloptes commented 1 month ago
Poster

Hi Michele, I cleaned up the changes.

The issue I had back then was that when I have to sync all notes it does save one by one. It takes a lot of time. This is what I was trying to work around with some of the changes. It could be saving all the notes at once, but I give it up for now and the patch looks clean and usable.

Have a look please.

regards

Hi Michele, I cleaned up the changes. The issue I had back then was that when I have to sync all notes it does save one by one. It takes a lot of time. This is what I was trying to work around with some of the changes. It could be saving all the notes at once, but I give it up for now and the patch looks clean and usable. Have a look please. regards
MicheleC commented 3 weeks ago
Owner

Hi Emanoil, sorry for the late reply, I have been on holiday for a while. I have not forgotten your work on this, don't worry. I will look at it in coming weeks, can't say for sure when due to heavy workload. But be assured it is on my todo list :smile:

Hi Emanoil, sorry for the late reply, I have been on holiday for a while. I have not forgotten your work on this, don't worry. I will look at it in coming weeks, can't say for sure when due to heavy workload. But be assured it is on my todo list :smile:
deloptes commented 3 weeks ago
Poster

Hi Michele,

nor problem at all. I am also very busy, but found some time recently to go through it and test it, so that the work gets finally done.

No problem, I see there are many more people working on trinity now and understandably you and Slavek are busy.

It would be good to have it merged at some point :). For the moment I can sync notes only on my older N9, but as it is a plugin in Syncevolution and works quite well (I tested the basic use cases with the new patch), I think it is worth closing this topic in the proper way. I will look into making Sailfish sync notes this year as well, so the work is not in vain.

Hi Michele, nor problem at all. I am also very busy, but found some time recently to go through it and test it, so that the work gets finally done. No problem, I see there are many more people working on trinity now and understandably you and Slavek are busy. It would be good to have it merged at some point :). For the moment I can sync notes only on my older N9, but as it is a plugin in Syncevolution and works quite well (I tested the basic use cases with the new patch), I think it is worth closing this topic in the proper way. I will look into making Sailfish sync notes this year as well, so the work is not in vain.
MicheleC requested changes 1 week ago
Looks good, some minor changes required
@@ -456,0 +463,4 @@
return m_lastmodified;
}

void KNote::setLastModified(const TQDateTime &dt)
MicheleC

This function (setLastModified) is not used and not part of the DCOP interface. We should either remove it or add it to the DCOP interface for a note.

This function (setLastModified) is not used and not part of the DCOP interface. We should either remove it or add it to the DCOP interface for a note.
@@ -66,0 +72,4 @@
/**
* Set the last modified field in the journal
*/
void setLastModified( const TQDateTime& dt );
MicheleC

This function (setLastModified) is not used and not part of the DCOP interface. We should either remove it or add it to the DCOP interface for a note.

This function (setLastModified) is not used and not part of the DCOP interface. We should either remove it or add it to the DCOP interface for a note.
@@ -645,3 +656,3 @@
return;
}
m_noteUidModify = journal->uid();
m_noteUidModify = journal->uid();
MicheleC

This is only spacing changes. should be omitted.

This is only spacing changes. should be omitted.
@@ -98,3 +100,3 @@
KNotesResourceManager *mManager;
TQDict<KNotesIconViewItem> mNoteList;
TQString mOldName;
TQString mOldName;
MicheleC

This is only spacing changes. should be omitted.

This is only spacing changes. should be omitted.
MicheleC commented 1 week ago
Owner

Hi Emanoil, PR looks good. Just need to clean up a few minor things as per comments in the code review. After you update those, the PR can be merged.

Could you also squash all the commits together into one? Thanks.

Hi Emanoil, PR looks good. Just need to clean up a few minor things as per comments in the code review. After you update those, the PR can be merged. Could you also squash all the commits together into one? Thanks.

Reviewers

MicheleC requested changes 1 week ago
This pull request can be merged automatically.
Sign in to join this conversation.
Loading…
Cancel
Save
There is no content yet.