Logger notifications to handle directory changes via config (#120) #128

Closed
wants to merge 8 commits into
from

Projects

None yet

3 participants

@dglol

Fixes #120.
Also included: change of styling (" -> ')

@whimboo

We don't want to let the logger handle the directory changes done by a preference. The one and only interface into the logger is the dir property. As mentioned earlier on another pull request any preference observer should stay in main. Given that it would need a larger rewrite of the patch I will stop commenting for now.

@dglol

I realized that but decided to move it into the logger.js due to the following cases:

  • A folder can be moved/deleted; this will cause the logger to fail silently
  • A user can just ignore the popup notification and the next time the browser starts up, the invalid pref will be the default directory and proceed to fail silently

There are ways to prevent this such as defaulting to the profile directory if the directory is invalid, but whether this is good as expected behavior is your decision.

@whimboo

Your first point is valid and the logger has to react accordingly. But it should better emit data and not open a notification on its own. It's really part of the application how to react on it.

Also we shouldn't default to the profile directory in all cases. It should only happen when we do not know the formerly set directory. That will happen on a start/activation. Otherwise we can check in the preferences handler if the new directory can be set, and if not we reset the preference to the formerly folder.

@dglol

I moved the popup notification to main.js

@whimboo whimboo commented on an outdated diff Apr 11, 2012
extension/lib/logger.js
@@ -62,10 +62,15 @@ Logger.prototype = {
catch (e) {
// Otherwise we also support a path
if (typeof(aValue) === 'string') {
- let dir = Cc['@mozilla.org/file/local;1']
- .createInstance(Ci.nsILocalFile);
- dir.initWithPath(aValue);
- this._dir = dir;
+ try {
+ let dir = Cc['@mozilla.org/file/local;1']
+ .createInstance(Ci.nsILocalFile);
+ dir.initWithPath(aValue);
+ this._dir = dir;
+ }
+ catch (e2) {
+ throw new Error('The path you have selected is invalid');
@whimboo
whimboo Apr 11, 2012

Can we use better naming for exception variables? Also please remove 'you' from the error message and just say 'The selected path is invalid'

@whimboo whimboo commented on the diff Apr 11, 2012
extension/lib/main.js
@@ -26,6 +27,12 @@ var gData = {
previous: {}
};
+var showNotification = function (aId, aMessage, aActions) {
+ let window = window_utils.activeBrowserWindow;
+ window.PopupNotifications.show(
+ window.gBrowser.selectedBrowser, aId, aMessage, null, aActions
+ );
@whimboo
whimboo Apr 11, 2012

Please use the notifications module here:
file:///data/code/addon-sdk/doc/packages/addon-kit/docs/notifications.html

@dglol
dglol Apr 11, 2012

I've looked at that module but I would not recommend it as it requires OS X to have Growl installed or else it will log the error silently to console.

@whimboo
whimboo Apr 11, 2012

Hm, that's bad. I wonder why the SDK doesn't make use of the popup notifications you are using. Would you mind asking?

@whimboo whimboo and 1 other commented on an outdated diff Apr 11, 2012
extension/lib/main.js
+ console.error(e.message);
+
+ let window = window_utils.activeBrowserWindow;
+ showNotification('logger_error', e.message, {
+ label: 'Select Path',
+ accessKey: 'S',
+ callback: function () {
+ let filePicker = Cc['@mozilla.org/filepicker;1']
+ .createInstance(Ci.nsIFilePicker);
+ filePicker.init(window, 'The directory where logs are stored',
+ Ci.nsIFilePicker.modeGetFolder)
+
+ let value = filePicker.show();
+ if (value === Ci.nsIFilePicker.returnOK) {
+ logger.dir = filePicker.file;
+ }
@whimboo
whimboo Apr 11, 2012

Do we really need all that code? Isn't the reverted value immediately visible in the addon settings? I don't think that we should come up with a modal dialog here. Simply resetting the pref to the last value if changes happen in about:config sounds totally fine for me, and there you can see that we revert the path directlly.

Also how do we react on startup if the dir is invalid? Currently we should fail because that's not handled.

@dglol
dglol Apr 11, 2012

There shouldn't be a case where it is invalid on startup that I am aware of. If the config and preferences are fail-safe then it shouldn't have a problem on startup.

However, deleting the folder after startup is a flaw, but handling it properly would involve checks in a couple places (turn on logging, open folder).

@whimboo
whimboo Apr 11, 2012
  1. Disable the extension
  2. Modify the log folder to an invalid path
  3. Enable the extension

Those steps will cause us to fail initially initializing the logger and resetting the pref.

@davehunt
Mozilla member

Testing this, I get a notification with the text:

Component returned failure code: 0x80520012
(NS_ERROR_FILE_ACCESS_DENIED)
[nsILocalFile.create]

Also, I am able to dismiss this notification and start logging, which will silently fail.

@dglol

I can't seem to reproduce that error Dave. What are the steps you took?

EDIT: Nevermind, I found the cause.

@dglol

Fixed davehunt's case but still one more to go.

@dglol

Forgot to mention that the startup case after disabling has been fixed. The solution isn't the most elegant, so criticism would be nice.

@whimboo whimboo and 1 other commented on an outdated diff May 23, 2012
extension/lib/logger.js
let dir = Cc['@mozilla.org/file/local;1']
.createInstance(Ci.nsILocalFile);
dir.initWithPath(aValue);
- this._dir = dir;
+ temp = dir;
+ }
+ catch (e2) {
@whimboo
whimboo May 23, 2012

Why e2? This can perfectly be 'e'

@dglol
dglol May 24, 2012

You're right. This used to be nested but not anymore.

@whimboo whimboo and 1 other commented on an outdated diff May 23, 2012
extension/lib/main.js
@@ -26,6 +27,35 @@ var gData = {
previous: {}
};
+// Notifies the user with a popup notification
+var showNotification = function (aId, aMessage, aActions) {
+ let window = window_utils.activeBrowserWindow;
+ window.PopupNotifications.show(
+ window.gBrowser.selectedBrowser, aId, aMessage, null, aActions
+ );
+};
+
+var notifyInvalidPath = function (logger, message) {
+ console.error(message);
@whimboo
whimboo May 23, 2012

I think that this was only for debugging purposes?

@dglol
dglol May 24, 2012

I left it to also have the error logged in the console but I'll revert this change.

@whimboo whimboo commented on an outdated diff May 23, 2012
extension/lib/main.js
+
+ let window = window_utils.activeBrowserWindow;
+ showNotification('logger_error', message, {
+ label: 'Select Path',
+ accessKey: 'S',
+ callback: function () {
+ let filePicker = Cc['@mozilla.org/filepicker;1']
+ .createInstance(Ci.nsIFilePicker);
+ filePicker.init(window, 'The directory where logs are stored',
+ Ci.nsIFilePicker.modeGetFolder)
+
+ let value = filePicker.show();
+ if (value === Ci.nsIFilePicker.returnOK) {
+ logger.dir = filePicker.file;
+ prefs.set(config.preferences.log_directory, logger.dir.path);
+ }
@whimboo
whimboo May 23, 2012

So I wouldn't show the file picker here. In case of invalid paths we should just show the popup and reset the path to the last known directory or fallback to the profile folder.

@dglol

New commit up with the mentioned changes.

Btw, Henrik: I tested the changes on the add-on sdk master branch and the notification does not appear -- in fact, I don't think the error is even caught. Do you know why this is?

@whimboo

Not sure if they have changed something for notifications. What does the 1.7 release do?

Also it looks like that the patch is not fully synced with the upstream master. Would be nice if you could update it.

@whimboo

David, can you please give us an update? If you do not have time please say so and someone else will pick this up. Thanks.

@dglol

I will definitelty try to make time for this. Was the only update you wanted a merge to upstream or is there more you need?

@whimboo

For now I want to test your change but can't do it as long it can't be applied. Once I was able to test I will let you know if everything works as expected or if an additional commit is necessary. Thanks!

@dglol dglol Merge branch 'master' into log-notification
Conflicts:
	extension/lib/main.js
2d2166d
@dglol

Merged master into this commit

@whimboo

Somehow this didn't worked out or another push made this invalid. The given branch is still not up-2-date :S

@whimboo

This PR hasn't been updated for a long time. If we want to continue I would propose to create a new PR.

@whimboo whimboo closed this Oct 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment