Proposal to extend the dcop interface for knotes #3

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

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,
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

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

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.

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.
In KNote::saveData(), I don't understand why the m_lastModified is not saved here but only later when saving the journal.
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.
some changes are only spaces/tabs and could be avoided to make the patch smaller to look at.
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 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 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 👍

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

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

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

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.

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.

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 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.

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

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

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

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

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,
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.

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 😉

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.

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

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 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 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.

I have not yet review the knotes_part of the PR

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.

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 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 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.

Looks good, some minor changes required

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 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 tested more and it is not good for some test cases.

Keep on hold please.

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

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.

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

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.

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 ```

Keep it up Emanoil, you will get there 😄

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.

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?

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.

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

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.

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 Emanoil, I think we should split the work into two steps.

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
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?

where is the DCOP interface
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.

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 :)

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 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 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.

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

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

img10.jpg

78 KiB

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 😄

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 Emanoil,

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

add getLastModified to DCOP interface
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.

First we add getLastModified() only, which will return the current last modified values, even if wrong. This can be easily merged.
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.

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 :)

Great, looking forward for the updated PR :-)

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

Hi Michele,

push forced d3b5286262 and created the issue with the workaround patch

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?

Finally merged!!!!

Halleluja, thank you!

Thank you for your good work Emanoil 😄

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

     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();

     TQDateTime d;
     if ( note )
         d = note->getLastModified();
     if (!d.isValid())

     /**
      * Set the last modified field in the journal
      */
     void setLastModified( const TQDateTime& dt );

          return;
   }
   m_noteUidModify = journal->uid();
     m_noteUidModify = journal->uid();

     KNotesResourceManager *mManager;
     TQDict<KNotesIconViewItem> mNoteList;
   TQString mOldName;
     TQString mOldName;

     m_journal->setSummary( m_label->text() );
     m_journal->setDescription( m_editor->text() );
     m_lastmodified = TQDateTime::currentDateTime();

 }
 TQDateTime KNote::getLastModified() const
 {

     return m_lastmodified;
 }
 void KNote::setLastModified(const TQDateTime &dt)

     TQDateTime dt;
     if ( note )
         dt = note->journal()->lastModified();
     if (!dt.isValid())