Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification support via libnotify #270

Merged
merged 6 commits into from
Dec 5, 2018

Conversation

norbusan
Copy link
Collaborator

@norbusan norbusan commented Dec 4, 2018

No description provided.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Makefile Show resolved Hide resolved
@abraunegg
Copy link
Owner

What happens in the event the 'notification' code is built into the application, but there ends up being no X / GDM or whatever .. like a headless server? Will the notifications just fire & not produce an error - or will some sort of error message somewhere be created/generated?

@norbusan
Copy link
Collaborator Author

norbusan commented Dec 4, 2018

What happens in the event the 'notification' code is built into the application, but there ends up being no X / GDM or whatever .. like a headless server? Will the notifications just fire & not produce an error - or will some sort of error message somewhere be created/generated?

The errors are try - catch protected and are vloged, so in normal operation mode nothing will happen, and in verbose mode the exception is dumped (probably one could extract only the relevant part of the exception and dump it).

The worst thing I have seen on my system is that on startup the first n.show() hangs for 30sec waiting for the dbus to come up (which it doesn't of course), but then continues without any problems.

So on a headless server the worst problem is that the first sync operation is delayed not 45sec but 45+30sec.

@abraunegg
Copy link
Owner

abraunegg commented Dec 4, 2018

Just tested this via CentOS 7.x

When running in monitor mode, and the following log (basically editing a local file via 'vi'):

Applying changes of Path ID: 01D6JQORV6Y2GOVW7725BZO354PWSELRRZ
[M] File changed: ./.1.txt.swx
Uploading differences of ./.1.txt.swx
Uploading new items of ./.1.txt.swx
./.1.txt.swx: No such file or directory
[M] Item deleted: ./.1.txt.swx
The item to delete is not in the local database
[M] File changed: ./.1.txt.swp
Uploading differences of ./.1.txt.swp
Uploading new items of ./.1.txt.swp
Uploading file ./.1.txt.swp ...
Uploading 100% |oooooooooooooooooooooooooooooooooooooooo|   ETA   --:--:--:
 done.
Remaining free space: 1099511378409
[M] Item deleted: ./.1.txt.swp
Deleting item from OneDrive: ./.1.txt.swp
[M] File changed: ./1.txt
Uploading differences of ./1.txt
Uploading new items of ./1.txt
Uploading file ./1.txt ...
Uploading 100% |oooooooooooooooooooooooooooooooooooooooo|   ETA   --:--:--:
 done.
Remaining free space: 1099511378396
[M] File changed: ./.1.txt.swp
Uploading differences of ./.1.txt.swp
Uploading new items of ./.1.txt.swp
./.1.txt.swp: No such file or directory
[M] Item deleted: ./.1.txt.swp
The item to delete is not in the local database

The last item above The item to delete is not in the local database lingered in the notification popup for quite a while. I think what we need to do is 'tweak' that error message

I think the whole issue comes from 'monitor' mode wanting to sync 'temporary' files that are created when files are opened localled / being edited whilst 'monitoring' that actual directory.

When moving a file into the monitored folder & is successfully uploaded, no notification message is presented that it was uploaded - eg: mv 3.txt ./OneDrive/

When downloading a new file from OneDrive, no notification message is presented that it was downloaded is presented.

Readme / man entry says - monitor mode will send notifications about initialization and errors via libnotify to the dbus

@norbusan
Copy link
Collaborator Author

norbusan commented Dec 4, 2018

The last item above The item to delete is not in the local database lingered in the notification popup for quite a while. I think what we need to do is 'tweak' that error message

Yes indeed. That comes from the monitor callback where the catch routine does do notify. This is a problem independent of notifications: short-lived files are tried to be uploaded immediately and when they disappear before being uploaded there is an error.

I will look into this in a different issue, no good reason to do this here.

When moving a file into the monitored folder & is successfully uploaded, no notification message is presented that it was uploaded - eg: mv 3.txt ./OneDrive/

Yes, as said, successful operation do not trigger a notification.

Readme / man entry says - monitor mode will send notifications about initialization and errors via libnotify to the dbus

Yes, and the problem you mention is an actual error and should be reported, only that onedrive should deal with this error in a better way, in the sense that it should not occur in the first place ;-)

@norbusan norbusan closed this Dec 4, 2018
@norbusan norbusan reopened this Dec 4, 2018
@norbusan
Copy link
Collaborator Author

norbusan commented Dec 4, 2018

Since you approve, are you fine with merging, should I do it or do you do it?

BTW, I don't want to mess with your style, so please let me know what your preferred modus operandi is:

  • concerning merge of pull request (reviewer or poster?)
  • concerning merge style (squashed commits or as is)

Thanks

@abraunegg
Copy link
Owner

I have been using "squash and merge" so that in the case of most PR's there have been a number of commit's into the PR itself - that way, the commit into master is a single commit - whilst larger, with the Travis CI testing / building and running I have going on, it reduces the risk of a commit causing compilation & execution failure.

It would be good to work out how to 'fix' the Travis CI shared key when building (which is why for everyone else's PR x64 build fails) so that when x64 is tested, the application tests I have currently run & execute for everyone as this tests upload / download & cases where bad characters have caused issues in the past.

Since you approve, are you fine with merging, should I do it or do you do it?

I dont mind if your doing this - the code has to get at least 1 review so that there is visibility - the more eye's that are on it, it get's improved & developing this is also not my day job either :)

@abraunegg abraunegg merged commit cc6cbf5 into abraunegg:master Dec 5, 2018
@norbusan norbusan deleted the notifications branch December 6, 2018 00:00
@norbusan
Copy link
Collaborator Author

norbusan commented Dec 6, 2018

@abraunegg Concerning the Travis CI shared key stuff, that is not possible: https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions

If you don't mind, I will create my development branch in your repository in the future, and create a PR from there. PR from the same repository have access to the env vars and testing should work.

I think I will

  • develop stuff and test it in my repo in a branch
  • if it is ready I push the same branch to your repo
  • create a PR from the branch in your repo

that way travis works.

Another thing concerning travis: why did you go for travis-ci.com? There is a limited number of builds as far as I see. OTOH, on travis-ci.org all is free for OSS projects.

@lock
Copy link

lock bot commented Jan 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Jan 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants