#3 Proposal to extend the dcop interface for knotes

Merged
MicheleC merged 1 commits from feat/knotes_interface_extention into master 8 months ago
deloptes commented 2 years 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 2 years 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 2 years 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 2 years ago
Owner

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

Thanks, I will look at merging this over the weekend.
MicheleC commented 2 years 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 2 years ago
deloptes commented 2 years 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 2 years 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 👍

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 😉

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 😉

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
MicheleC left a comment

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 10 months 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 10 months 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 😄

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 10 months 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 9 months ago
MicheleC left a comment

Looks good, some minor changes required

knotes/knote.cpp
@@ -456,0 +463,4 @@
return m_lastmodified;
}

void KNote::setLastModified(const TQDateTime &dt)
MicheleC commented 9 months ago

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.
knotes/knote.h
@@ -66,0 +72,4 @@
/**
* Set the last modified field in the journal
*/
void setLastModified( const TQDateTime& dt );
MicheleC commented 9 months ago

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.
knotes/knotesapp.cpp
@@ -645,3 +656,3 @@
return;
}
m_noteUidModify = journal->uid();
m_noteUidModify = journal->uid();
MicheleC commented 9 months ago

This is only spacing changes. should be omitted.

This is only spacing changes. should be omitted.
kontact/plugins/knotes/knotes_part.h
@@ -98,3 +100,3 @@
KNotesResourceManager *mManager;
TQDict<KNotesIconViewItem> mNoteList;
TQString mOldName;
TQString mOldName;
MicheleC commented 9 months ago

This is only spacing changes. should be omitted.

This is only spacing changes. should be omitted.
MicheleC commented 9 months 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.
deloptes commented 9 months ago
Poster

Hi Michele,
I rebased on current master and squashed all commits into one.

I also removed setLastModified, compiled and tested. All seems to look good now.

pls review

regards

Hi Michele, I rebased on current master and squashed all commits into one. I also removed setLastModified, compiled and tested. All seems to look good now. pls review regards
deloptes commented 9 months ago
Poster

Hi Michele,

I tested more and it is not good for some test cases.

Keep on hold please.

Hi Michele, I tested more and it is not good for some test cases. Keep on hold please.
MicheleC commented 9 months ago
Owner

Hi Emanoil, ok I will wait. Can you share more info on the bad test cases?

Hi Emanoil, ok I will wait. Can you share more info on the bad test cases?
deloptes commented 9 months ago
Poster

It was updating the modified date each time it is loading the notes, resulting in unnecessary updates.

I moved the update of the field into the saveData function only when called with update=true.

Now I tested create/modifiy/delete notes on both sides, but I want to test it a bit more, because I am now not 100% sure.

I will update you when done.

It was updating the modified date each time it is loading the notes, resulting in unnecessary updates. I moved the update of the field into the saveData function only when called with update=true. Now I tested create/modifiy/delete notes on both sides, but I want to test it a bit more, because I am now not 100% sure. I will update you when done.
MicheleC commented 9 months ago
Owner

ok thanks. Let me know when you want me to retake a look.

ok thanks. Let me know when you want me to retake a look.
deloptes commented 9 months ago
Poster

Unfortunately I already forgot why there was so much work around this lastModified and especially the revision map I originally introduced.

I now started over and found out why it was necessary.

KNotes uses the KCal::Journal which inherits from Incidence, which has lastModified field.

Unfortunately this field is modified each time a note is opened or closed by knotesapp (setText(), setName()). This is why it was necessary to keep track of the revisions (lastModified) in the revision map I added earlier and rewrite to initial value when closing note (saveAll()) if not modified. Else you get updates for all notes even if they are not modified.

How should we proceed, because it looks like this tricking with the revision map was a workaround of this issue described above.

Perhaps it should be corrected in knotes and knotesapp, so that it does not modify the content of a note if content actually did not change (setText(), setName()).

I will try this next and come back. If you have another ideas let me know.

Unfortunately I already forgot why there was so much work around this lastModified and especially the revision map I originally introduced. I now started over and found out why it was necessary. KNotes uses the KCal::Journal which inherits from Incidence, which has lastModified field. Unfortunately this field is modified each time a note is opened or closed by knotesapp (setText(), setName()). This is why it was necessary to keep track of the revisions (lastModified) in the revision map I added earlier and rewrite to initial value when closing note (saveAll()) if not modified. Else you get updates for all notes even if they are not modified. How should we proceed, because it looks like this tricking with the revision map was a workaround of this issue described above. Perhaps it should be corrected in knotes and knotesapp, so that it does not modify the content of a note if content actually did not change (setText(), setName()). I will try this next and come back. If you have another ideas let me know.
deloptes commented 9 months ago
Poster

Hi, I spent almost whole free time today looking into the code. It is bad. So bad that I do not see any other way except my workaround with the lastModifiedMap to reset the last modified before saving.

I looked briefly and Michael Brade brade@kde.org (the last maintainer) was student at that time. What should I say :/ perhaps I am even worse than he was.

Now back to the problem. It sets the lastModified when loading the notes from the ics file in resouroce manager. I added debug to all functions to see the process. In recourcelocal.cpp I also did following with the output below.

I’ll try to find out where and why exactly it is resetting the modified field and stop it if possible.

Such a mess really!


bool ResourceLocal::load()
{
    kdDebug(5500) << k_funcinfo << endl;
    mCalendar.load( mURL.path() );

    KCal::Journal::List notes = mCalendar.journals();
    KCal::Journal::List::ConstIterator it;
    for ( it = notes.constBegin(); it != notes.constEnd(); ++it ) {
        kdDebug(5500) << (*it)->lastModified() << endl;
        manager()->registerNote( this, *it );
        kdDebug(5500) << (*it)->lastModified() << endl;
    }

    return true;
}

[knotes] Fri Feb 21 00:09:22 2020
[knotes] [void KNotesResourceManager::registerNote(ResourceNotes*, KCal::Journal*)]
[knotes] [void KNotesApp::createNote(KCal::Journal*)]
[knotes] [KNote::KNote(TQDomDocument, KCal::Journal*, TQWidget*, const char*)]
[knotes] [void KNote::setName(const TQString&)]
[knotes] [void KNote::updateLabelAlignment()]
[knotes] [virtual bool KNote::event(TQEvent*)]
[knotes] [void KNote::slotApplyConfig()]
[knotes] [void KNote::setColor(const TQColor&, const TQColor&)]
[libkcal] [void KCal::IncidenceBase::setLastModified(const TQDateTime&)]
[libkcal] [void KCal::IncidenceBase::setLastModified(const TQDateTime&)]
[libkcal] [void KCal::IncidenceBase::setLastModified(const TQDateTime&)]
[knotes] [TQString KNote::noteId() const]
[knotes] [virtual bool KNote::event(TQEvent*)]
[knotes] [void KNote::updateBackground(int)]
[knotes] [void KNote::createFold()]
[knotes] [void KNote::updateFocus()]
[knotes] [void KNote::updateLabelAlignment()]
[knotes] [void KNote::slotUpdateShowInTaskbar()]
[knotes] [void KNote::slotUpdateReadOnly()]
[knotes] [void KNote::updateFocus()]
[knotes] [void KNote::slotUpdateKeepAboveBelow()]
[knotes] [virtual bool KNote::event(TQEvent*)]
[knotes] [TQString KNote::noteId() const]
[knotes] Fri Feb 21 00:34:20 2020
Hi, I spent almost whole free time today looking into the code. It is bad. So bad that I do not see any other way except my workaround with the lastModifiedMap to reset the last modified before saving. I looked briefly and Michael Brade <brade@kde.org> (the last maintainer) was student at that time. What should I say :/ perhaps I am even worse than he was. Now back to the problem. It sets the lastModified when loading the notes from the ics file in resouroce manager. I added debug to all functions to see the process. In recourcelocal.cpp I also did following with the output below. I'll try to find out where and why exactly it is resetting the modified field and stop it if possible. Such a mess really! ``` bool ResourceLocal::load() { kdDebug(5500) << k_funcinfo << endl; mCalendar.load( mURL.path() ); KCal::Journal::List notes = mCalendar.journals(); KCal::Journal::List::ConstIterator it; for ( it = notes.constBegin(); it != notes.constEnd(); ++it ) { kdDebug(5500) << (*it)->lastModified() << endl; manager()->registerNote( this, *it ); kdDebug(5500) << (*it)->lastModified() << endl; } return true; } [knotes] Fri Feb 21 00:09:22 2020 [knotes] [void KNotesResourceManager::registerNote(ResourceNotes*, KCal::Journal*)] [knotes] [void KNotesApp::createNote(KCal::Journal*)] [knotes] [KNote::KNote(TQDomDocument, KCal::Journal*, TQWidget*, const char*)] [knotes] [void KNote::setName(const TQString&)] [knotes] [void KNote::updateLabelAlignment()] [knotes] [virtual bool KNote::event(TQEvent*)] [knotes] [void KNote::slotApplyConfig()] [knotes] [void KNote::setColor(const TQColor&, const TQColor&)] [libkcal] [void KCal::IncidenceBase::setLastModified(const TQDateTime&)] [libkcal] [void KCal::IncidenceBase::setLastModified(const TQDateTime&)] [libkcal] [void KCal::IncidenceBase::setLastModified(const TQDateTime&)] [knotes] [TQString KNote::noteId() const] [knotes] [virtual bool KNote::event(TQEvent*)] [knotes] [void KNote::updateBackground(int)] [knotes] [void KNote::createFold()] [knotes] [void KNote::updateFocus()] [knotes] [void KNote::updateLabelAlignment()] [knotes] [void KNote::slotUpdateShowInTaskbar()] [knotes] [void KNote::slotUpdateReadOnly()] [knotes] [void KNote::updateFocus()] [knotes] [void KNote::slotUpdateKeepAboveBelow()] [knotes] [virtual bool KNote::event(TQEvent*)] [knotes] [TQString KNote::noteId() const] [knotes] Fri Feb 21 00:34:20 2020 ```
MicheleC commented 9 months ago
Owner

Keep it up Emanoil, you will get there 😄

Keep it up Emanoil, you will get there :smile:
deloptes commented 9 months ago
Poster

One said “you can make only shit out of shit” so why not apply the workaround? I know it is not nice, but my motivation is very low on that issue.

One said "you can make only shit out of shit" so why not apply the workaround? I know it is not nice, but my motivation is very low on that issue.
MicheleC commented 9 months ago
Owner

I am a bit lost. Is the code in the PR ready for review or do you still want to do some work on it?

I am a bit lost. Is the code in the PR ready for review or do you still want to do some work on it?
SlavekB commented 9 months ago
Owner

As I understand, the code in PR is not really a solution to the core of the problem, but a workaround. At the same time, @deloptes found that examining the core of the problem is challenging, so the question arises whether to leave the core of the problem unresolved or to examine it in depth.

As I understand, the code in PR is not really a solution to the core of the problem, but a workaround. At the same time, @deloptes found that examining the core of the problem is challenging, so the question arises whether to leave the core of the problem unresolved or to examine it in depth.
MicheleC commented 9 months ago
Owner

I can take a look after I am done with bug 2929, unless Emanoil wants to do more work on it first.

I can take a look after I am done with bug 2929, unless Emanoil wants to do more work on it first.
deloptes commented 9 months ago
Poster

Thank you Slavek, I meant the original patch that Michele rejected was a workaround.

I made a clean patch based on our understanding, but found out that knotes is modifying the timestamp each time it loads the notes (without being actually modified). This is a serios bug that was worked around originally, but I forgot why.

I would push a patch based on the original workaround and ask you to review again.

The more I am looking into the code, the more agression I am building up :\

The original patch worked around the issues in knotes perfectly well for the past 8-9y for me.

Thank you Slavek, I meant the original patch that Michele rejected was a workaround. I made a clean patch based on our understanding, but found out that knotes is modifying the timestamp each time it loads the notes (without being actually modified). This is a serios bug that was worked around originally, but I forgot why. I would push a patch based on the original workaround and ask you to review again. The more I am looking into the code, the more agression I am building up :\ The original patch worked around the issues in knotes perfectly well for the past 8-9y for me.
deloptes commented 9 months ago
Poster

Hi,
almost there (with very small new workaround).

I have to go through knotes_part as well and test. Most probably start of next week will push.

Hi, almost there (with very small new workaround). I have to go through knotes_part as well and test. Most probably start of next week will push.
MicheleC commented 9 months ago
Owner

Hi Emanoil, I think we should split the work into two steps.

  1. add support for last modified date to DCOP. This should be pretty easy and it would be a subset of this PR. I expect that to be easily and quick to merge
  2. there is clearly a problem in how the last modified field is currently managed in TDE (for example knotes update the field each times we close it, even if a note was not modified). We should address this as a separate issue and PR.

    What do you think?
Hi Emanoil, I think we should split the work into two steps. 1. add support for last modified date to DCOP. This should be pretty easy and it would be a subset of this PR. I expect that to be easily and quick to merge 2. there is clearly a problem in how the last modified field is currently managed in TDE (for example knotes update the field each times we close it, even if a note was not modified). We should address this as a separate issue and PR.<br/> What do you think?
deloptes commented 9 months ago
Poster
  1. where is the DCOP interface

  2. yes it looks like a bug which got me very angry, cause it looks like a spagetti code - the work around is to reset the timestamp/last modified in registerNote

void KNotesResourceManager::registerNote( ResourceNotes *resource,
    KCal::Journal *journal )
{
    // TODO: only emit the signal if the journal is new?
    TQDateTime dt = journal->lastModified();
    m_resourceMap.insert( journal->uid(), resource );
    emit sigRegisteredNote( journal );
    journal->setLastModified(dt);
}

Somehow I am happy that I did rework this PR as now it is very clean.

And yes I agree that handling the issue 2. should be done in another PR.

  1. knotes(app) when closing overwrites notes created by kpart.

Why kpart is not using the knotesapp interface?

Or better - both should be using a single interface

         dcop_iface ----> resource
         /       \
        /         \
   knotesapp   kpart_knotes

do you think such thing is possible?

This will require alot of work - you know better, you can not make ravioly or lasagna out of spagetti :)

1. where is the DCOP interface 2. yes it looks like a bug which got me very angry, cause it looks like a spagetti code - the work around is to reset the timestamp/last modified in registerNote ``` void KNotesResourceManager::registerNote( ResourceNotes *resource, KCal::Journal *journal ) { // TODO: only emit the signal if the journal is new? TQDateTime dt = journal->lastModified(); m_resourceMap.insert( journal->uid(), resource ); emit sigRegisteredNote( journal ); journal->setLastModified(dt); } ``` Somehow I am happy that I did rework this PR as now it is very clean. And yes I agree that handling the issue 2. should be done in another PR. 3. knotes(app) when closing overwrites notes created by kpart. Why kpart is not using the knotesapp interface? Or better - both should be using a single interface ``` dcop_iface ----> resource / \ / \ knotesapp kpart_knotes ``` do you think such thing is possible? This will require alot of work - you know better, you can not make ravioly or lasagna out of spagetti :)
MicheleC commented 9 months ago
Owner

Hi Emanoil, I like the idea of a single DCOP interface for knotes. In that case I think it should be something like knotesapp using kpart_knotes which in turns expose the DCOP interface. Anyhow we can leave this for a later stage, no point in complicating this PR even more.

Would be good if we can address point 1. in this PR first, which would expose the last modified field of a note (even if wrongly modified). This should be very quick to do.

Then we can look at the problem in knotes and kontact (#16) and see what is the best way to solve the wrong update of the last modified field, possibly without work arounds.

Does it sound good for you?

Hi Emanoil, I like the idea of a single DCOP interface for knotes. In that case I think it should be something like knotesapp using kpart_knotes which in turns expose the DCOP interface. Anyhow we can leave this for a later stage, no point in complicating this PR even more. Would be good if we can address point 1. in this PR first, which would expose the last modified field of a note (even if wrongly modified). This should be very quick to do. Then we can look at the problem in knotes and kontact (#16) and see what is the best way to solve the wrong update of the last modified field, possibly without work arounds. Does it sound good for you?
deloptes commented 9 months ago
Poster

Hi Michele,

I have a problem with this. I added in KNotesIface.h. I see the knotes_part uses this interface, but I do not know what to do, to complete your request.

Wasn’t there a way to generate some files from the interface header?

thanks

Hi Michele, I have a problem with this. I added in KNotesIface.h. I see the knotes_part uses this interface, but I do not know what to do, to complete your request. Wasn't there a way to generate some files from the interface header? thanks
MicheleC commented 9 months ago
Owner

Hi Emanoil,

for point 1. (extending DCOP interface) it would be enough to get the part of the PR related to the new getLastModified() functions. Don’t worry if the field is incorrectly updated each time knotes is closed, we can work o nthat later.

If you want I can create PR to show you what I mean.

Hi Emanoil,<br/> for point 1. (extending DCOP interface) it would be enough to get the part of the PR related to the new getLastModified() functions. Don't worry if the field is incorrectly updated each time knotes is closed, we can work o nthat later. If you want I can create PR to show you what I mean.
deloptes commented 9 months ago
Poster

Michele, I really do not understand what you want me to do - if you can, please show.

Michele, I really do not understand what you want me to do - if you can, please show.
deloptes commented 9 months ago
Poster

I open kdcop and navigate to knotes - expand and navigate to KNotesIface - double click getLastModified() and provide notes UID. When executed it returns the last modification time.

I think what you are requesting is already there

I open kdcop and navigate to knotes - expand and navigate to KNotesIface - double click getLastModified() and provide notes UID. When executed it returns the last modification time. I think what you are requesting is already there
MicheleC commented 9 months ago
Owner

I open kdcop and navigate to knotes - expand and navigate to KNotesIface - double click getLastModified() and provide notes UID. When executed it returns the last modification time.

I think what you are requesting is already there

Hi Emanoil,

don’t forget this PR has not been merged yet, so “getLastModified()” is not in the master code. So on a stardard TDE system, there is no “getLastModified()” in KNotesIface yet.

What I am asking in point 1. is exactly that: make a PR with the code to add “getLastModified()” to the interface only, so we can merge it. The code is a subset of this PR. If still unclear, I will create the PR myself 😄

> I open kdcop and navigate to knotes - expand and navigate to KNotesIface - double click getLastModified() and provide notes UID. When executed it returns the last modification time. > > I think what you are requesting is already there Hi Emanoil,<br/> don't forget this PR has not been merged yet, so "getLastModified()" is not in the master code. So on a stardard TDE system, there is no "getLastModified()" in KNotesIface yet.<br/> What I am asking in point 1. is exactly that: make a PR with the code to add "getLastModified()" to the interface only, so we can merge it. The code is a subset of this PR. If still unclear, I will create the PR myself :smile:
deloptes commented 9 months ago
Poster

Hi,

but this PR covers already the KNotesIface. This PR is about adding getLastModified() to knotes. and kpart is in knotes. Why should it be in a separate PR?

The name of this PR is mentioning DCOP anyway

I can do whatever I can in the evening.

Hi, but this PR covers already the KNotesIface. This PR is about adding getLastModified() to knotes. and kpart is in knotes. Why should it be in a separate PR? The name of this PR is mentioning DCOP anyway I can do whatever I can in the evening.
MicheleC commented 9 months ago
Owner

Hi Emanoil,

sorry for not explaining well. My understanding is that this PR is trying to do two things:

  1. add getLastModified to DCOP interface
  2. fix the problem with wrong update of that field (like for example in knotes application).

For point 2. we discussed at length that we are not sure of the correct solution and we want to further investigate. So I previously suggested to split the two things.

  1. First we add getLastModified() only, which will return the current last modified values, even if wrong. This can be easily merged.
  2. Then we will investigate the problem of wrong updating separately, different PR.

What I am asking is to get the subset of this PR that only related to the getLastModified(), without the code for remembering/updating the actual value.

Hi Emanoil,<br/> sorry for not explaining well. My understanding is that this PR is trying to do two things: 1. add getLastModified to DCOP interface 2. fix the problem with wrong update of that field (like for example in knotes application). For point 2. we discussed at length that we are not sure of the correct solution and we want to further investigate. So I previously suggested to split the two things. 1. First we add getLastModified() only, which will return the current last modified values, even if wrong. This can be easily merged. 2. Then we will investigate the problem of wrong updating separately, different PR. What I am asking is to get the subset of this PR that only related to the getLastModified(), without the code for remembering/updating the actual value.
deloptes commented 9 months ago
Poster

Aaah OK, I now understand fully what you mean.

I will push --force without the workaround ... or do a separate commit to not forget again what was the trick :)

Aaah OK, I now understand fully what you mean. I will push --force without the workaround ... or do a separate commit to not forget again what was the trick :)
MicheleC commented 9 months ago
Owner

Great, looking forward for the updated PR :-)

Great, looking forward for the updated PR :-)
deloptes commented 9 months ago
Poster

Hi,
sorry I hit a NFS bug and had to update the kernel on the machines - I will do this evening (hopefully) :)

Hi, sorry I hit a NFS bug and had to update the kernel on the machines - I will do this evening (hopefully) :)
deloptes commented 9 months ago
Poster

Hi Michele,

push forced d3b5286262 and created the issue with the workaround patch

Hi Michele, push forced d3b5286262 and created the issue with the workaround patch
MicheleC reviewed 8 months ago
MicheleC left a comment

Thanks Emanoil, PR is good. Can you clarify what was your intention with the return date in case a note is not find?
Did you want to return an invalid date or a pre-nitialized date to Jan 1970?

knotes/knotesapp.cpp
@@ -345,0 +348,4 @@
TQDateTime d;
if ( note )
d = note->getLastModified();
if (!d.isValid())
MicheleC commented 8 months ago

Same as commented here

Same as commented [here](https://mirror.git.trinitydesktop.org/gitea/TDE/tdepim/pulls/3/files#issuecomment-6322)
kontact/plugins/knotes/knotes_part.cpp
@@ -277,0 +280,4 @@
TQDateTime dt;
if ( note )
dt = note->journal()->lastModified();
if (!dt.isValid())
MicheleC commented 8 months ago

Comment in the code is “else return invalid date”. In such case there is no need to use setTime_t(), since the object “dt” is created as an invalid date already. In fact using “setTime_t()” creates a valid date, although in 1970

Comment in the code is "else return invalid date". In such case there is no need to use setTime_t(), since the object "dt" is created as an invalid date already. In fact using "setTime_t()" creates a valid date, although in 1970
deloptes commented 8 months ago

I was thinking the same - but left as it is. I think somewhere was a problem with invalid datetime, so the comment is not exactly correct, but is meant that you will see start of epoch 1970...

Could be that is obsolete, because AFAIR some bugs were fixed in the date time. I can not say more to this.

I was thinking the same - but left as it is. I think somewhere was a problem with invalid datetime, so the comment is not exactly correct, but is meant that you will see start of epoch 1970... Could be that is obsolete, because AFAIR some bugs were fixed in the date time. I can not say more to this.
MicheleC commented 8 months ago

How about we remove those two lanes and return an empty invalid date? If later we run into any issue witn invalid dates somewhere else, let’s fix those in the right place, rather than returning a valid date but set 50 years ago 😉

You can either update the commit or I can do that after merging this one, just let me.

How about we remove those two lanes and return an empty invalid date? If later we run into any issue witn invalid dates somewhere else, let's fix those in the right place, rather than returning a valid date but set 50 years ago :wink:<br/> You can either update the commit or I can do that after merging this one, just let me.
deloptes commented 8 months ago

No objections, I can’t do it now - need to follow the family program at Saturday :/

No objections, I can't do it now - need to follow the family program at Saturday :/
MicheleC commented 8 months ago

Ok, no problem. I will update your commit and merge. You will still be credited as author but the commit will get my GPG signature though...

Ok, no problem. I will update your commit and merge. You will still be credited as author but the commit will get my GPG signature though...
MicheleC closed this pull request 8 months ago
MicheleC deleted branch feat/knotes_interface_extention 8 months ago
MicheleC commented 8 months ago
Owner

Finally merged!!!!

Finally merged!!!!
MicheleC added this to the R14.1.0 release milestone 8 months ago
MicheleC removed the
PR/wip
label 8 months ago
deloptes commented 8 months ago
Poster

Halleluja, thank you!

Halleluja, thank you!
MicheleC commented 8 months ago
Owner

Thank you for your good work Emanoil 😄

now let’s get the other part of the task done!

Thank you for your good work Emanoil :smile:<br/> now let's get the other part of the task done!

Reviewers

MicheleC requested changes 9 months ago
The pull request has been merged as 094f8d9a87.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.