Confirmed.

DnD seems not affected. Just the move-to-trash from the right-mouse-click menu in konq

I don't have the time into look into it, but here is the essence of the issue

In konq_popupmenu.cc it reads the value for Empty

    if ( isCurrentTrash )
    {
        act = new TDEAction( i18n( "&Empty Trash Bin" ), "emptytrash", 0, TQT_TQOBJECT(this), TQT_SLOT( slotPopupEmptyTrashBin() ), &m_ownActions, "empytrash" );
        KSimpleConfig trashConfig( "trashrc", true );
        trashConfig.setGroup( "Status" );
        act->setEnabled( !trashConfig.readBoolEntry( "Empty", true ) );
        addAction( act );
    }

It looks like that it does not always update the value for Empty, which should be done here in fileAdded

trashimpl.cpp
	void TrashImpl::fileAdded()
	{
	m_config.setGroup( "Status" );
	if ( m_config.readBoolEntry( "Empty", true ) == true ) {
		m_config.writeEntry( "Empty", false );
		m_config.sync();
	}
	// The apps showing the trash (e.g. kdesktop) will be notified
	// of this change when KDirNotify::FilesAdded("trash:/") is emitted,
	// which will be done by the job soon after this.
	}
	
	void TrashImpl::fileRemoved()
	{
	if ( isEmpty() ) {
		m_config.setGroup( "Status" );
		m_config.writeEntry( "Empty", true );
		m_config.sync();
	}
	// The apps showing the trash (e.g. kdesktop) will be notified
	// of this change when KDirNotify::FilesRemoved(...) is emitted,
	// which will be done by the job soon after this.
	}

Which gives the question why it works only the first time.
If you restore the file and delete it, it works again.

regards

Created attachment 2752 [details]
good when trash is updated


In successful scenario there is notification emitted RemoteDirNotify::toRemoteURL(trash:/), which would end in an update- trashrc is not updated (emtpy remains true).
I attach the good and the bad scenario logs, just to keep in mind where I was and perhaps soneone can explain why it looks so different.


media tdeioslave: ()
remote tdeioslave: RemoteDirNotify::FilesAdded
remote tdeioslave: RemoteDirNotify::toRemoteURL(system:/users/emanoil/test/)
remote tdeioslave: result => KURL()
media tdeioslave: MediaDirNotify::toMediaURL(trash:/)
media tdeioslave: MediaList::list()
media tdeioslave: ()
remote tdeioslave: RemoteDirNotify::FilesAdded
remote tdeioslave: RemoteDirNotify::toRemoteURL(trash:/)
remote tdeioslave: result => KURL()
tdeio_uiserver: UIServer::jobFinished id=31



media tdeioslave: ()
remote tdeioslave: RemoteDirNotify::FilesAdded
remote tdeioslave: RemoteDirNotify::toRemoteURL(system:/users/emanoil/test/)
remote tdeioslave: result => KURL()
tdeio_uiserver: UIServer::totalFiles 34 0
tdeio_uiserver: UIServer::totalDirs 34 0
tdeio_uiserver: UIServer::processedDirs 34 0
tdeio_uiserver: UIServer::jobFinished id=34

Created attachment 2753 [details]
bad when trash is not updated

when I kill all tdeio_trash, the icon/status is displayed correctly

so the conclusion to this is that each time tdeio: [bool TrashImpl::init()] is called, it displays the state correctly. When the tdeio_trash process is running, it does not alter the state correctly.

I narrowed it down to TrashImpl::fileAdded() when called it reads the status of .trinity/share/config/trashrc

[Status]
Empty=true

if Empty=true it should write Empty=false and sync, but either this does not happend or somehow gets rewritten. after printing the status of Empty in such case it shows "false", but in the file it is still "true" - so the update of the icon correctly represents the status of the value in the config file.

... to be continued

Created attachment 2835 [details]
A simple solution to reflect the status - the price is an overhead

isEmpty() is called in fileRemoved, while in fileAdded it first checks the status of the config file value which reports Empty=false, however in the file itself the value is Empty=true, thus it never updates the config file to reflect the state.
if we use isEmpty() in the latter case it adds an overhead to check the state, but does not relay on the status reported by m_config.readBoolEntry( "Empty", true )

regards

Created attachment 2836 [details]
A simple solution to reflect the status - the price is an overhead

cosmetic change to previous patch and 
m_config.setGroup( "Status" ); placed in the condition

Created attachment 2837 [details]
A simple solution to reflect the status - the price is an overhead

must have been working late, I missed the negation, however testing worked and I just noticed when looking at the diff

Created attachment 2838 [details]
A simple solution to reflect the status

Great job thanks Emanoil!.
Slavek or I will confirm and push it when we have the time, hopefully I can do that this Sunday.

thanks for the good word, but I am not proud of it as it does not resolve the root cause, but offers a stupid work around.

I am unable to understand why this happens - perhaps we discuss it on gmane

Emanoil, 
I have been doing some analysis on this as a temporary break-away from bug 2874. I ran into a difference of behavior in case the trash is emptied from the desktop icon popup menu or from the konqueror popup menu. This is why you see funny things with the Empty entry in trashrc. Basically there are two trash processes running, one associated to the desktop trash icon and one related to the konqueror action. Seems like that when emptying the trash from the desktop the value of the empty entry is not sync the way it should be before performing the action.
I need to spend some more time on this, probably this weekend I think.

Hi,
thanks for looking into this. I am sorry I did not mention this earlier.
Here is my test scenario

- Move the file to trash using konqueror-right click->Move to Trash
- Double click trash icon - opens konqueror
- right click on file and select restore
- Move the file to trash using konqueror-right click->Move to Trash
- icon is not updated because in trashrc it is still Empty=true, but m_config returns false in the if conditon and prevents from writing. As soon as it is written in the config I guess the notification (KDirNotify) kicks in and desktop gets updated - the icon is changed. 

This boils down to the same issue (IMO) as the original test scenario. Comment 3 was based on testing the original scenario. However the scenario mentioned above is much easier to play with.

just add a line to print m_config.readBoolEntry( "Empty" ) before the if statement and try both scenarios.

I also understood that there are too processes running for desktop and trash bin, but couldn't follow.

regards

>thanks for the good word, but I am not proud of it as it does not resolve
>the root cause, but offers a stupid work around.

Actually your patch is not a stupid work at all, it is quite good.

The problem is caused by the fact that there are different tdeio_trash processes running at the same time and all writing to the same trashrc file. Depending on the order the processes are created and the trashrc file is updated, problems could or could not arise.
Example: 
- first process is created from konqueror when trashing a file, with initially an empty trash bin. So the Empty property is initially true and after the file is trashed, Empty=false.  
- clicking on the desktop trash icon opens another konqueror, creating another trash process. Here Empty=false. Now restore the file. Empty becomes true again.
- trash another file from the original konqueror process, this will not update the trashrc value since this process already thinks the trash is not empty from the first trashed file. Result is that Empty=true but the trash bin is empty.

The above is just one of the possible situations, there are many more.

Your patch makes sure to check the real status of the trash bin when a file is added, so the status is consistent no matter how many trash processes there are and the order of their creation and operations.

In fact it is not even necessary to call isEmpty() since after adding a file to the trash bin, it will never be empty and the result of the call would be always false.
R14.1: commit f88a8e57
R14.0: commit ac79ba37

A proper solution would be to have a DCOP-based config object, then the status of the config could be shared across different processes and the updated value checked on the spot, which would prevent the problem in first place.


Could you and Q4OS team check the above commit is ok? I will wait your feedback before closing the bug.

> Result is that Empty=true but the trash bin is empty.

Meant to be:
  Result is that Empty=true but the trash bin is NOT empty.

thanks - the point checking the value of Empty is to reduce subsequent writes.
with current patch it will write to the file each time something is added to the trashbin, so my patch is not pretty clever and also suggesting that if something is added its not empty is better but still resulting in extra write.

I still think there should be a better solution

> thanks - the point checking the value of Empty is to reduce subsequent writes.
> with current patch it will write to the file each time something is added to 
> the trashbin, so my patch is not pretty clever and also suggesting that if 
> something is added its not empty is better but still resulting in extra write.
Yes, with both your patch or mine, there is a writing to the trashrc file every time a file is added. This is necessary to make sure the status of the trashbin icon is consistent, to prevent multiple tdeio_trash processes to write (or not write) the correct value. The problem comes from the fact that when a process updates the value of Emtpy, the other processes are not notified of the new value.

> I still think there should be a better solution
Yes, there is. The solution is to have a DCOP-style config object, where the value of the entries are shared across multiple processes using DCOP (or even DBUS). Then when a process accesses the Empty property, it will always read the current value (except in cases of unlikely concurrent accesses).
The point is that it would take quite a bit of time to develop something like that, and given we are already extremely short of time it may not be worth the effort. Writing to trashrc when we trash a file is not a huge overhead overall.
Moreover, using such solution would be applicable for R14.1.x onwards but not for R14.0.x because it would require an API and ABI change.
I discussed with Slavek and we agreed that for the time being the easy solution that you suggested is very good and clean and also can be backported into R14.0.x.
If in future we ever find the time to develop such DCOP-stype TDE config object,  we can revisit this bug in R14.1.x series.

Q4OS team, Emanoil, could you check if with the latest commit everything is working fine?
I will wait your feedback before closing the bug.

Looks good, I am not able to reproduce it after the latest TDE-14.0.5 update. Debian 9 Stretch, TDE 14.0.5, tdebase-trinity 4:14.0.5~pre23-0debian9.0.1.+4

Thanks Michele, you can close

Great thanks.
As mentioned in one of the comments, if we ever find the time to develop a DCOP-based TDE shared config object, we shall revisit this bug.
Closing for the time being.