Closed Bug 343416 Opened 18 years ago Closed 17 years ago

implement nsIIdleService - API to get the last time user activity occurred on the system

Categories

(Core :: Widget, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 7 obsolete files)

Having an API as described in the summary is useful for IM applications and other things where behaviour should be influenced in some way by the user (not) being around. 

As for where it goes, I have no idea if there are existing interfaces this could go on. Getting some enlightenment from more widget-knowledgeable people would be great.

As for implementation, GAIM seems to be of help here:
http://svn.sourceforge.net/viewcvs.cgi/gaim/trunk/src/gtkidle.c?view=markup

Uses IOKit for Mac, some windows api's, and xscreensaver and/or gtk if on *nix, afaict. I'm not sure how much of this can be done, but I'd think gtk and win32 should be fine, not sure about mac or xscreensaver.
This could be as simple as having a couple of new observer topics (for use with nsIObserverService):

idle-userinterface
active-userinterface

The former would be triggered after some predefined period of UI inactivity. The second would be fired when the UI is "reactivated" by some user activity (mouse or keyboard).

The one issue I see with this is that you can't configure how long you want the idle period to be. Personally I wouldn't have an issue with making this a global preference, at least in the initial implementation.
This is probably really wrong, and such. But it's my first try, and it does in fact seem to work quite well here. Please do not be sparing in critique, I'll try and fix things until most people are happy :-)

The patch adds an nsIActivityService interface, which has one single method that allows you to ask for the time since the last user activity. Its linux implementation silently assumes "on the current display", and because, for whatever reason, in xpcshell GDK_DISPLAY() returns null, I'm nullchecking that and failing if it's not there.

I think that's mostly all there is to it. I can try writing a windows implementation when I return to Amsterdam (and have access to a windows machine again) January 2.
Assignee: general → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #249845 - Flags: superreview?(neil)
Attachment #249845 - Flags: review?(roc)
Comment on attachment 249845 [details] [diff] [review]
Tentative patch adding interface + linux implementation using xscreensaver

>+ * The Original Code is Mozilla Communicator.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corp.
>+ * Portions created by the Initial Developer are Copyright (C) 1999
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Gijs Kruitbosch
I find it hard to think that you needed Netscape's help to author this ;-)

>+    void getTimeSinceLastActivity ( out unsigned long timePassed );
JavaScript really doesn't like you doing this:
var timePassed = {};
activityService.getTimeSinceLastActivity(timePassed);
var dateOfLastActivity = new Date(Date.now() - timePassed.value);
The following two .idl definitions generate the same C++, but are easier to use from JavaScript:
>unsigned long getTimeSinceLastActivity();
var timePassed = activityService.getTimeSinceLastActivity();
var dateOfLastActivity = new Date(Date.now() - timePassed);
>readonly attribute unsigned long timeSinceLastActivity;
var timePassed = activityService.timeSinceLastActivity;
/* again, no .value needed */

>+ * Portions created by the Initial Developer are Copyright (C) 1998
This licence was almost right ;-)

>+NS_IMPL_THREADSAFE_ADDREF(nsActivityService)
>+NS_IMPL_THREADSAFE_RELEASE(nsActivityService)
>+
>+NS_INTERFACE_MAP_BEGIN(nsActivityService)
>+    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIActivityService)
>+    NS_INTERFACE_MAP_ENTRY(nsIActivityService)
>+NS_INTERFACE_MAP_END_THREADSAFE
NS_IMPL_THREADSAFE_ISUPPORTS1(nsActivityService, nsIActivityService)

>+nsActivityService::nsActivityService()
>+{}
>+
>+nsActivityService::~nsActivityService()
>+{}

>+    nsActivityService();
>+    virtual ~nsActivityService();
If you really need to explicitly declare these then you might as well inline them (include the body in the .h instead of the .cpp); also I doubt that the destructor needs to be virtual.

sr=me with these fixed.
Attachment #249845 - Flags: superreview?(neil) → superreview+
Attached patch Better patch (obsolete) — Splinter Review
Changes:
* Everything Neil said (as far as I can tell)
* Make nsIdleService implement an nsIObserver and use it to:
** Allow people to add/remove observers that get notified once when we're longer away than N minutes (data parameter for the observer), and notified again if we're no longer that long away.
** Catch callbacks from a timer we start when we have an observer added, and remove when we no longer have any observers (so we don't bother the system without any need for it).
* More comments.

Things I worry about:
* Do I need to free the struct I get from xss? Calling XFree on it after getting it (as the docs suggest one should) makes glibc crash and burn, claiming I tried to free something twice. Hohum.
* Does the static declaration of that struct matter? Why?
* Do I need to delete all the leftover elements in the two nsVoidArrays in my destructor? Why (not)?
* How would I properly test for the presence of Xscreensaver in configure.in / wherever?
* Doesn't this implementation work for GTK+ (as opposed to GTK2) as well? Does that mean it should be moved/copied?
* I would really like at least comments in the idl about the observer stuff, but I'm not sure how that'd work - I think it would be preferable if users of the interface used the observer stuff if they used it more than once, so as to minimize system strain (if everyone starts polling it themselves, that might lag - never tried it, but I just consider the observer system nicer). Anyway, I think timeless said I shouldn't specify the nsIObserver stuff in the idl, so I didn't, and that made me unsure of how/where/whether I should add comments about all that.
* How badly my lack of C++-fu is affecting the quality of this patch :-)

OK, that's about it, I believe. Review/complain away. :)
Attachment #249845 - Attachment is obsolete: true
Attachment #249924 - Flags: superreview?(roc)
Attachment #249924 - Flags: review?(neil)
Attachment #249845 - Flags: review?(roc)
Comment on attachment 249924 [details] [diff] [review]
Better patch

Some comments, even though I'm relatively new to this myself.

Consider adding unit tests for this service - it's a good practice in general and makes it easier for others to figure out how to use the service.

Also some comments as to how this service work wouldn't hurt.

>+interface nsIIdleService : nsISupports
>+{
It would make more sense to expose the addListener/removeListener methods on this interface than making users register themselves via the observe() method.

>+struct IdleListener {
Why not class?

>+    IdleListener(nsCOMPtr<nsIObserver> obs, PRUint32 reqIT)
You shouldn't pass nsCOMPtr as parameter, just use nsIObserver*

For detailed discussion see http://www.mozilla.org/projects/xpcom/nsCOMPtr.html

>+static nsVoidArray* arrayHere = 0;
>+static nsVoidArray* arrayIdle = 0;

I think these ought to be members of the service, not pointers, and no need for nsVoidArray. You could use
 nsTArray<IdleListener*> arrayXXX;
or better
 nsTArray< nsAutoPtr<IdleListener> > arrayXXX;
for automatic releases.

See http://developer.mozilla.org/en/docs/XPCOM:Arrays#nsTArray.3CT.3E for more info. I'm not sure why you need two arrays either.

>+static nsCOMPtr<nsITimer> myTimer = nsnull;
>+static PRUint32 lastIdleTime = 0;

same, this could be class members. Also just |timer| instead of myTimer.

>+        if (mit_info == nsnull)

Just |if (!mit_info)|. The jst-review script is pretty good at catching these :)

>+    return NS_OK;

As I said on IRC, shouldn't this be NS_ERROR_FAILURE in case the function call above fails?

>+    // Check if this is a timer callback:
>+    if (!strncmp(NS_TIMER_CALLBACK_TOPIC, topic, 15))

magic number 15? http://lxr.mozilla.org/seamonkey/search?string=NS_TIMER_CALLBACK_TOPIC appears to suggest using strcmp instead.

>+    {
>+        nsCOMPtr<nsITimer> timer(do_QueryInterface(observer));
>+        if (!timer)
>+            return NS_ERROR_NO_INTERFACE;

Why do you need this?

>+    // Get a real observer

I don't think this code belongs here, see the beginning of the comment.

>+nsresult nsIdleService::AddListener(nsCOMPtr<nsIObserver> observer, const PRUnichar *data)

Same as above, just use nsIObserver* as the parameter. If this becomes a method on the activityservice interface, you can make the second parameter an integer and avoid the conversion code below.

Also check if you get a NULL observer, you're assuming it's not null later.

>+    IdleListener *listener = new IdleListener(observer, time);

Check for OOM (!listener).

>+nsresult nsIdleService::RemoveListener(nsCOMPtr<nsIObserver> observer, const PRUnichar *data)

Same as for AddListener.

>+    // We check for an entry with this observer and this time in both our
>+    // arrays, and remove it if we find it.

Are you sure that the users need to register the same observer with different idle time?

Also, isn't it possible to use the array's builtin method for removal instead?

>+    nsAutoString timeStr;
>+    timeStr.AppendInt(idleTime);
>+
I think users can get the idle time directly from the service.
(In reply to comment #4)
> * Do I need to delete all the leftover elements in the two nsVoidArrays in my
> destructor? Why (not)?

You need to, but better use nsTArray< nsAutoPtr<...> > as I suggested.

> * I would really like at least comments in the idl about the observer stuff,
> but I'm not sure how that'd work - I think it would be preferable if users of
> the interface used the observer stuff if they used it more than once, so as to
> minimize system strain (if everyone starts polling it themselves, that might
> lag - never tried it, but I just consider the observer system nicer). Anyway, I
> think timeless said I shouldn't specify the nsIObserver stuff in the idl, so I
> didn't, and that made me unsure of how/where/whether I should add comments
> about all that.

I suggested
  void addAwayObserver(in unsigned long idleTime, in nsIObserver observer);
on IRC yesterday, seems much cleaner than registering via observe()
Summary: Would like an API to get the last time user activity occurred on the system → implement nsIIdleService - API to get the last time user activity occurred on the system
Comment on attachment 249924 [details] [diff] [review]
Better patch

>+ * Portions created by the Initial Developer are Copyright (C) 2007 
LOL @ 2007 but I'm still nitting you for the trailing space :-P

>+#include "nsIObserver.idl"
You don't use this one... yet... but you could forward-declare it, i.e.
interface nsIObserver;

>+    IdleListener(nsCOMPtr<nsIObserver> obs, PRUint32 reqIT)
Don't pass nsCOMPtr parameters, just use nsIObserver*

>+    {
>+        observer = obs;
>+        reqIdleTime = reqIT;
>+    }
C++ has this handy (and efficient) "initialise the members" syntax:
IdleListener(nsIObserver *obs, PRUint32 reqIT)
  : observer(obs),
    reqIdleTime(reqIT) {}

>+static nsVoidArray* arrayHere = 0;
>+static nsVoidArray* arrayIdle = 0;
>+static nsCOMPtr<nsITimer> myTimer = nsnull;
>+static PRUint32 lastIdleTime = 0;
You're not supposed to have a static nsCOMPtr, as you can't guarantee its destruction. However as you only have one nsIdleServer you can make all these member variables and the nsCOMPtr will clear itself when XPCOM destroys your service. You might even find one of bsmedberg's nsTArray templates will clean up after your IdleListeners for you.

>+    // Check if this is a timer callback:
>+    if (!strncmp(NS_TIMER_CALLBACK_TOPIC, topic, 15))
>+    {
>+        nsCOMPtr<nsITimer> timer(do_QueryInterface(observer));
>+        if (!timer)
>+            return NS_ERROR_NO_INTERFACE;
>+        CheckAwayState();
>+        return NS_OK;
>+    }
I didn't know about this version of nsITimer; I'd have used initWithCallback

>+    nsresult rv;
>+    if (!strncmp("add-idle-listener", topic, 18))
>+        rv = AddListener(real_observer, data);
>+    else if (real_observer && !strncmp("remove-idle-listener", topic, 21))
>+        rv = RemoveListener(real_observer, data);
>+    else
>+        return NS_ERROR_FAILURE;
>+
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    return NS_OK;
Well, interesting logic there, but I think you would find it easier to scrap this and add addListener and removeListener methods to nsIIdleService.

>+    // Make sure our observer goes into 'idle' immediately if applicable.
>+    CheckAwayState();
I'm not sure this works... if you've just recreated a timer, your last idle time might be wrong.

>+            if (myTimer && (arrayHere->Count() + arrayIdle->Count() == 0))
My first idea was to compare the counts to zero separately (so if the here count is nonzero then there's no point checking the idle count), but rather than duplicating the code I would add a flag member to IdleListener.

>+            curListener = (IdleListener*)arrayHere->ElementAt(i);
I can't remember which one but I think one of these array types might have a FastElementAt which doesn't bounds-check i (if you're feeling lucky).
Attachment #249924 - Flags: review?(neil) → review-
(In reply to comment #5)
> (From update of attachment 249924 [details] [diff] [review])
> Consider adding unit tests for this service - it's a good practice in general
> and makes it easier for others to figure out how to use the service.
Now, I don't pretend to really understand unit tests very well - but isn't the concept you run them without a full browser, preferably without a window, if possible? As one of the comments mentions, for lack of a GDK_DISPLAY, the entire thing fails to work properly without a window, as far as I can tell - it fails in xpcshell at the very least.
Also, it would seem hard/impossible to somehow generate activity that makes the OS think the user did something - if the OS doesn't differentiate between user-provided and program-provided input signals for its idle time then that's a bug in a lot of cases, I believe. So I'm not quite sure how one would test this in a unit test.

> Also some comments as to how this service work wouldn't hurt.
What exactly do you mean by 'work' (ie, internally? usage?), and where would these comments go? One of my mentioned problems is that I would like to add comments, but am unsure where they would go. I'll be happy to write stuff for DevMo if this is finished - but as far as I can tell that'll be a while longer. 

> >+    // We check for an entry with this observer and this time in both our
> >+    // arrays, and remove it if we find it.
> 
> Are you sure that the users need to register the same observer with different
> idle time?
It seems like that's the most likely case, if one user wants to get notifications for different idle times. Compare screensaver/monitor/harddisk settings on most OSes, which let you give separate times at which each should shutdown. I don't see why it shouldn't be possible, anyway. It doesn't actually add much more code nor require extra parameters everywhere - you need to know the interval anyway to be sure you can notify a user on idle.

> Also, isn't it possible to use the array's builtin method for removal instead?
Not as far as I can tell, given that you deduce an object's time from a string, therefor use a new PRUint32, and a new struct, which, assuming the builtin method does a 'dumb' pointer compare, will mean it won't find the object you construct from the data that you get, because it's not the same object as the thing in the array.

> >+    nsAutoString timeStr;
> >+    timeStr.AppendInt(idleTime);
> I think users can get the idle time directly from the service.

Yes, but that's one more xpcom/xpconnect call, and as far as I can tell the alternative is leaving the data param empty (that is, I can't think of any other useful things the caller would care about). We already have the actual data, all we need to do is put it in a string - it seemed like a waste not to, really.

Thanks for all the technical comments, I snipped them because I'll have to address them in a patch, not a comment, as far as I can tell. :) It'll be a day or two before I'm able to do that, though.

One more thing: the class vs. struct thing - why a class and not a struct, really? If there's a good argument to use either, I'll be glad to use them. Not to blame him, but when I discussed the array usage and how I'd do that in xpcom, timeless said I could use nsVoidArray's and pointers to struct keeping the time and observer, so that's how I did it. I'm not really sure why I would want a class and not a struct, or the reverse.
Attached patch Patch v3 (obsolete) — Splinter Review
I think I've addressed the review comments with this patch:
* I'm using a flag on the IdleListener class now, not two arrays
* I'm refcounting the class so the implementation copes as properly as I can manage with removing elements from the observer's observe method itself.
* I'm using nsTArray<nsRefPtr<IdleListener> >* for the array, which means that it should cope with deleting the IdleListeners just by removing them from the array, I believe :-).
* I've added the addIdleObserver and removeIdleObserver to the idl, and removed the nsIObserver inheritance.
* I think that by now, I really do need the nsIObserver include in the idl, so it's still there.
* I'm using the initWithFuncCallback version of nsITimer now.
* I'm cleaning up the timer when the service is destroyed, in case it hasn't been.
And just because Neil mentioned it, I left in the (c) 2007 bit (though I removed the trailing space) - I somehow doubt this will get r+sr and checkin within the next few hours :-)

Also, I'd really like some input on the configure.in and xscreensaver build stuff. Roc, could you help with that or do I need to CC bsmedberg as well?
Attachment #249924 - Attachment is obsolete: true
Attachment #250060 - Flags: superreview?(neil)
Attachment #250060 - Flags: review?(roc)
Attachment #249924 - Flags: superreview?(roc)
Comment on attachment 250060 [details] [diff] [review]
Patch v3

>+     * Removing an observer for some idle time will not remove it for all other
>+     * idle times.
Literally "not ... all" means "none or some". You can probably remove the word "all" completely, or you can rephrase it as "not ... any other idle time."
[I'm also not convinced that you need to support multiple times per observer.]

>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsIdleService, nsIIdleService)
I'm not convinced this is true.

>+    timer = nsnull;
Unnecessary, as timer is an nsCOMPtr which knows to init itself to null. (And remember this is not how you init a member anyway).

>+    // We check for an entry with this observer and this time in our array, and
>+    // remove it if we find it.
>+    for (; i < arrayListeners->Length(); i++)
>+    {
>+        nsRefPtr<IdleListener> curListener = arrayListeners->ElementAt(i);
>+        if (curListener->reqIdleTime == time &&
>+            curListener->observer == observer)
There is actually an IndexOf method that may or may not be helpful.

>+        {
>+            arrayListeners->RemoveElementAt(i);
>+            // clear our timer if there's no longer any need for it.
>+            if (timer && (arrayListeners->Length() == 0))
IsEmpty()

>+    nsTArray< nsRefPtr<IdleListener> >* listeners = arrayListeners;
This just copies the pointer to the array, it doesn't clone the array.

>+        if (--refCount == 0)
>+            delete this;
>+        return refCount;
You can't return refCount after you deleted this.

>+    nsTArray< nsRefPtr<IdleListener> > *arrayListeners;
This should be a member, not a pointer.
Attachment #250060 - Flags: superreview?(neil) → superreview-
(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 249924 [details] [diff] [review] [details])
> > Consider adding unit tests for this service - it's a good practice in general
> > and makes it easier for others to figure out how to use the service.
> Now, I don't pretend to really understand unit tests very well - but isn't the
> concept you run them without a full browser, preferably without a window, if
> possible? As one of the comments mentions, for lack of a GDK_DISPLAY, the
> entire thing fails to work properly without a window, as far as I can tell - it
> fails in xpcshell at the very least.

No, you don't have to run it without a browser. sayrer's mochitest thing runs in the browser, so do reftest testcases.

> Also, it would seem hard/impossible to somehow generate activity that makes the
> OS think the user did something - if the OS doesn't differentiate between
> user-provided and program-provided input signals for its idle time then that's
> a bug in a lot of cases, I believe. So I'm not quite sure how one would test
> this in a unit test.
> 
Point, but at least provide the code you use to test this. (Note that some of the logic can still be tested automatically, but I don't think it's fair to make you write code with 100% test coverage :) )

Now that you have a sane interface, it's not as important though...

> > Also some comments as to how this service work wouldn't hurt.
> What exactly do you mean by 'work' (ie, internally? usage?), and where would
> these comments go?

I was talking about internal code, but now it's simpler than in the previous revision.

> > >+    nsAutoString timeStr;
> > >+    timeStr.AppendInt(idleTime);
> > I think users can get the idle time directly from the service.
> 
> Yes, but that's one more xpcom/xpconnect call, and as far as I can tell the
> alternative is leaving the data param empty (that is, I can't think of any
> other useful things the caller would care about). We already have the actual
> data, all we need to do is put it in a string - it seemed like a waste not to,
> really.
> 
You spend time on converting the time to string and the user needs to parse it 
back into an integer. Seems unclean to me.

We discussed the rest of points on irc, I believe.

re attachment 250060 [details] [diff] [review]:
"Behavior for adding the same observer twice with the same idle time is undefined."
I think it should be made a no-op (or a failure).

In nsIdleService::AddIdleObserver !listener should result in NS_ERROR_OUT_OF_MEMORY and for observer you could use NS_ENSURE_ARG_POINTER (and for time NS_ENSURE_ARG). Returning NS_ERROR_FAILURE in all cases sometimes makes it harder to figure out what exactly a method didn't like.
Attached patch Patch v4 (obsolete) — Splinter Review
Changes:
* Better error messages (thanks asqueella)
* Added a comparator for idlelisteners so I could use RemoveElement and IndexOf
* Adding the same observer for the same idle time is now a no-op, using the same comparator w/ IndexOf
* No longer claim to be threadsafe
* Fixed (I think) all the nits Neil pointed out.
Attachment #250060 - Attachment is obsolete: true
Attachment #250094 - Flags: superreview?(neil)
Attachment #250094 - Flags: review?(roc)
Attachment #250060 - Flags: review?(roc)
Comment on attachment 250094 [details] [diff] [review]
Patch v4

Sorry about not mentioning this earlier:

>+#include "nsIObserver.idl"

I believe you still can forward-declare nsIObserver instead of using #include.

>+[scriptable, uuid(cc52f19a-63ae-4a1c-9cc3-e79eace0b471)]
>+interface nsIIdleService : nsISupports
>+{
>+    /**
>+     * The amount of time passed since the last user activity.

in milliseconds/seconds/minutes/hours?

>+     */
>+    readonly attribute unsigned long idleTime;
>+
>+    /**
>+     * Add an observer to be notified when the user idles for some period of
>+     * time, and when they get back from that.
>+     * @param observer the observer to be notified

You should document what parameters will be passed to observer's observe() method. See nsIPrefBranch2 for example.

>+     * @param time the amount of time the user should idle before the observer
>+     *             should be notified.

Same comment as for idleTime.

>+     * @note
>+     * You can add the same observer twice for different idle times. Adding the

Like Neil, I'm not convinced it's really needed. The downside of doing this is that the user is required to remember the time that observer was registered for, as well as the observer object itself.

>+     * same observer with the same idle time is a no-op.
>+     */
>+    void addIdleObserver(in nsIObserver observer, in unsigned long time);
>+

You should probably document the behavior in case the user is idle when the observer is registered.

Also, you have hardcoded 5000 as the polling interval. I think it should be minimum of registered idle times, and the |time| param should be checked to be >100 ms.

>+    /**
>+     * Remove an observer that used to be notified when the user idled for
>+     * some period of time, and when they got back from that.

Maybe "Remove an observer registered using addIdleObserver"?

>+nsIdleService::~nsIdleService()
>+{
>+    arrayListeners.Clear();

~nsTArray calls Clear()

>+NS_IMETHODIMP nsIdleService::GetIdleTime(PRUint32 *aTimeDiff)

Sometimes you have the NS_IMETHODIMP / return type on the same line as the rest of the declaration, sometimes on different line - be consistent.

It looks like this is the only platform-specific method and the rest of code can be shared across platforms.

>+        if (!mit_info)
>+            mit_info = XScreenSaverAllocInfo();
>+        XScreenSaverQueryInfo(dplay, GDK_ROOT_WINDOW(), mit_info);
comment 4:
> Do I need to free the struct I get from xss? Calling XFree on it after
> getting it (as the docs suggest one should) makes glibc crash and burn,
> claiming I tried to free something twice. Hohum.

Are you absolutely sure you're doing everything right? Did you try consulting xscreensaver folks and/or filing a bug? Also I'd make mit_info a class member rather than a static.

>+        *aTimeDiff = (mit_info->idle);

Parentheses are not needed.

>+nsIdleService::AddIdleObserver(nsIObserver *observer, PRUint32 time)
>...
>+    NS_ENSURE_ARG(time);

See above, I think it should be time > SOME_CONSTANT.

>+    arrayListeners.AppendElement(listener);
>+
This can fail due to OOM.

>+void nsIdleService::CheckAwayState()
>...
>+        if ((curListener->reqIdleTime * 60000 <= idleTime) &&

So in one method, time is measured in milliseconds, and in another - in minutes? Confusing, I'd say.

>+            curListener->observer->Observe(this, "idle", timeStr.get());

It's common practice to prefix the topic with your name, e.g. http-, em-, timer-, ipc-.

Maybe also use a #define for the topic string.

>+    IdleListener(nsIObserver* obs, PRUint32 reqIT, PRBool aIsIdle = PR_FALSE) :
aObserver, aRequiredIdleTime, aIsIdle

>+    PRUint32 refCount;

Normally member fields are prefixed with "m".

>+    nsCOMPtr<nsITimer> timer;
>+    nsTArray< nsRefPtr<IdleListener> > arrayListeners;

Same.
+     * @note
+     * You can add the same observer twice for different idle times. Adding the
+     * same observer with the same idle time is a no-op.

I don't think it should be a no-op. That would be a quirky API. Imagine if two different pieces of code both happen to add the same observer with the same idle time and one mysteriously doesn't work. Another way of looking at this is that addObserver(obs, t); removeObserver(obs, t); should always be a no-op but in some contexts, with this API, it isn't.
Comment on attachment 250094 [details] [diff] [review]
Patch v4

Looks mostly OK, except for these minor tweaks:

>+    arrayListeners.Clear();
~nsTArray() calls Clear(); for you.

>+    PRUint32 i = 0;
>+    nsTArray< nsRefPtr<IdleListener> > listeners = arrayListeners;
>+    for (; i < listeners.Length(); i++)
Have you been reading the C++ portability guidelines? It doesn't actually make any difference here, so you might as well declare for (PRUint32 i = 0;

>+// Refcounted class  we can use to store an observer with its associated
Nit: two spaces between class and we

>+    NS_IMETHODIMP_(nsrefcnt)
Not sure that the NS_IMETHODIMP_() stuff is actually needed here. Anyone?
Attachment #250094 - Flags: superreview?(neil) → superreview+
(In reply to comment #15)
> >+    NS_IMETHODIMP_(nsrefcnt)
> Not sure that the NS_IMETHODIMP_() stuff is actually needed here. Anyone?

I don't think it's needed.
Attached patch Patch v5 (obsolete) — Splinter Review
Of course, the instant I get sr+, I go and make more radical changes so as to appease everyone, including Nickolay, Matthew, Gavin and who knows who else.

Either way:
* Split up the class into an XP part and a platform-specific bit. It'll still only be a "real" service on gtk2 right now, but it'll be easier to write windows/mac/insert-your-favourite-toolkit-or-os-here implementations. All you need to do is add some makefile and component registration stuff, and write a method that gets the time since the last user interaction in milliseconds.
Moving along, I believe I've fixed all of Neil's nits, I've changed the double observer + idletime case to just throw back NS_ERROR_FAILURE instead (thanks roc!), and I've fixed most of the stuff asqueella pointed at, also XFree'ing the struct I keep around in the destructor. It doesn't crash in this case. What else... Oh, right. asqueella pointed out that AppendElement can fail due to OOM. But I didn't know how to check for that, so I've left it as-is.

That's about it, I believe. Oh, right, I'd still like some input on the linking with xscreensaver and accompanying configure.in checks. So I'll be CC-ing bsmedberg next. :-)
Attachment #250094 - Attachment is obsolete: true
Attachment #250237 - Flags: superreview?(neil)
Attachment #250237 - Flags: review?(roc)
Attachment #250094 - Flags: review?(roc)
Why don't you make duplicate observer+idletime just work?
Comment on attachment 250237 [details] [diff] [review]
Patch v5

>+/**
>+ * This interface lets you monitor how long the user has been 'idle',
>+ * ie not used their mouse or keyboard. You can get the idle time directly,
"i.e."

>+nsIdleServiceGTK::nsIdleServiceGTK()
>+{
>+ mMitInfo = nsnull;

nsIdleServiceGTK::nsIdleServiceGTK()
  : mMitInfo(nsnull)
see comment 7.

>+nsIdleServiceGTK::GetIdleTime(PRUint32 *aTimeDiff)
>...
>+if (!mMitInfo)
>+  mMitInfo = XScreenSaverAllocInfo();

You should check for OOM here.

>+ nsresult GetIdleTime(PRUint32* idleTime);

NS_IMETHOD?

>+    nsresult AddIdleObserver(nsIObserver* aObserver, PRUint32 aIdleTime);
>+    nsresult RemoveIdleObserver(nsIObserver* aObserver, PRUint32 aIdleTime);

NS_IMETHOD, add a comment indicating these methods come from nsIIdleService.

>+    void CheckAwayState();

This could be protected.


Also, from the little I know and remember about C++, nsIdleServiceGTK::~nsIdleServiceGTK doesn't get called with this patch (right?)

You should make your destructors virtual or maybe make them private and do the NS_IMPL_ISUPPORTS thing for the most derived classes.
Attached patch Patch v6 (obsolete) — Splinter Review
Patch with (I believe) everything Nickolay and Robert mentioned fixed. Fortunately, allowing multiple observers with the same idle time wasn't hard, as far as I can tell.

Oh, right, I've chosen to solve the destructor issue by declaring/implementing nsISupports on the derived class, making the destructors private (and protected on the base class, so it can actually get called from the derived class). I've tested this with some printf debugging, it seems to work just fine. :-).
Attachment #250237 - Attachment is obsolete: true
Attachment #250333 - Flags: superreview?(neil)
Attachment #250333 - Flags: review?(roc)
Attachment #250237 - Flags: superreview?(neil)
Attachment #250237 - Flags: review?(roc)
Comment on attachment 250333 [details] [diff] [review]
Patch v6

>+    nsresult rv;
>+    rv = GetIdleTime(&idleTime);
>+    if (NS_FAILED(rv))
>+        return;
It occurs to me that you might want to write this as
nsresult rv = GetIdleTime(&idleTime);
or even
if (NS_FAILED(GetIdleTime(&idleTime))
Attachment #250333 - Flags: superreview?(neil) → superreview+
(In reply to comment #21)
> (From update of attachment 250333 [details] [diff] [review])
> >+    nsresult rv;
> >+    rv = GetIdleTime(&idleTime);
> >+    if (NS_FAILED(rv))
> >+        return;
> It occurs to me that you might want to write this as
> nsresult rv = GetIdleTime(&idleTime);
> or even
> if (NS_FAILED(GetIdleTime(&idleTime))

Is there a reason not to use NS_ENSURE_SUCCESS?
+nsIdleService::~nsIdleService()
+{
+    if (mTimer)
+    {
+        mTimer->Cancel();
+        mTimer = nsnull;
+    }

mTimer = nsnull; is not needed here, since you're in the destructor.

Why heap-allocate the IdleListener objects? Can't you just have them directly in the array?
Hmm... some of the xpwidgets files seem to be named nsBaseFoo or nsXPFoo but there's no consistency :-(

(In reply to comment #23)
>+nsIdleService::~nsIdleService()
>+{
>+    if (mTimer)
>+    {
>+        mTimer->Cancel();
>+        mTimer = nsnull;
>+    }
>mTimer = nsnull; is not needed here, since you're in the destructor.
I can't believe I saw the nsTArray.Clear() and missed this one...

>Why heap-allocate the IdleListener objects? Can't you just have them directly
>in the array?
I believe he wants to copy the list in case it mutates during enumeration.
I think we should flatten the list, make IdleListener non-refcounted, and modify CheckAwayState so that it walks through the list updating the IdleListeners and adding any notifications that need to be fired to two nsCOMArrays. Then they get fired safely.
Attached patch Patch v7 (obsolete) — Splinter Review
New patch:
* Does what roc said in the comment above, I believe.
* Dynamically loads xscreensaver functions so we won't die on machines without it (tested by moving libXss.* out of my PATH and running the same binary, that worked (well, failed, but didn't crash)), and also removes the linking dep on it, meaning it should compile and run on current tinderboxen. I think. :)
* Use a one-line NS_FAILED and NSPR logging to log if things go wrong when adding an observer. Not sure if I got the NSPR logging entirely right...

I think that's all.
Attachment #250333 - Attachment is obsolete: true
Attachment #250584 - Flags: superreview?(neil)
Attachment #250584 - Flags: review?(roc)
Attachment #250333 - Flags: review?(roc)
Comment on attachment 250584 [details] [diff] [review]
Patch v7

>+    xsslib = PR_LoadLibrary("libXss.so");
>+    if (!xsslib) // ouch.
>+        return;

I'd log a warning in this case.

>+    _XSSQueryExtension = (_XScreenSaverQueryExtension_fn)
>+        PR_FindFunctionSymbol(xsslib, "XScreenSaverQueryExtension");
>+    _XSSAllocInfo = (_XScreenSaverAllocInfo_fn)
>+        PR_FindFunctionSymbol(xsslib, "XScreenSaverAllocInfo");
>+    _XSSQueryInfo = (_XScreenSaverQueryInfo_fn)
>+        PR_FindFunctionSymbol(xsslib, "XScreenSaverQueryInfo");

...and here.

>+    if (xsslib)
>+        PR_UnloadLibrary(xsslib);


>+NS_IMETHODIMP
>+nsIdleServiceGTK::GetIdleTime(PRUint32 *aTimeDiff)

I'd add failure logging here instead of the general (and useless) warning in nsIdleService.

>+// interval in milliseconds between internal idle time requests to xscreensaver

remove the mention of xscreensaver, this code will be used for other platforms too

>+        IdleListener curListener = mArrayListeners.ElementAt(i);

I think you can do
 IdleListener& curListener = ...
and avoid the ReplaceElementsAt call later.

>Index: src/xpwidgets/nsIdleService.h
>...
>+#include "nsAutoPtr.h"

Do you still need this?

>+// Refcounted class .....
>+class IdleListener {
not anymore?

>+    // Implement nsIIdleService methods, but not the idletime getter,
"idleTime" or "idle time"?
Attached patch Patch v8Splinter Review
OK, fixed everything Nickolay pointed at, I believe. Using NSPR logging in the gtk2 part now, tested that, it seems to work if you know what to do (that took me a while, but anyway...). No more ReplaceElementAt calls, I've tested things again and it looks like this works fine, too. Fixed all the spelling nits and such... hopefully this is it. :-)
Attachment #250584 - Attachment is obsolete: true
Attachment #250604 - Flags: superreview?(neil)
Attachment #250604 - Flags: review?(roc)
Attachment #250584 - Flags: superreview?(neil)
Attachment #250584 - Flags: review?(roc)
Comment on attachment 250604 [details] [diff] [review]
Patch v8

>+            PR_LOG(sIdleLog, PR_LOG_WARNING, ("One of the Xss functions is missing!\n"));
Didn't you log this already?

>+        return (a.observer == b.observer) &&
>+                (a.reqIdleTime == b.reqIdleTime);
Nit: indentation is wrong - (s should line up.
(Or remove the ()s and then the a.s should line up.)
Attachment #250604 - Flags: superreview?(neil) → superreview+
Comment on attachment 250604 [details] [diff] [review]
Patch v8

+    ((nsIdleService*)aClosure)->CheckAwayState();

NS_STATIC_CAST
Attachment #250604 - Flags: review?(roc) → review+
So, two nits of my own:
* Actually, forward-declaring nsIObserver breaks building this stuff. It claims the a.observer == b.observer is trying to construct a struct nsIObserver and it doesn't know how to do that and... well, a fair amount of other build errors which magically go away if I go back to #include-ing nsIObserver.
* I need to look for the libXss.so.1 library - the generic one tends to get symlinked by the -dev packages for xscreensaver on most distros, meaning it's not there on user machines.
Checked in. Condensed log follows.

Checking in public/Makefile.in;
new revision: 1.109; previous revision: 1.108
Checking in public/nsIIdleService.idl;
initial revision: 3.1
Checking in public/nsWidgetsCID.h;
new revision: 3.49; previous revision: 3.48
Checking in src/gtk2/Makefile.in;
new revision: 1.61; previous revision: 1.60
Checking in src/gtk2/nsIdleServiceGTK.cpp;
initial revision: 1.1
Checking in src/gtk2/nsIdleServiceGTK.h;
initial revision: 1.1
Checking in src/gtk2/nsWidgetFactory.cpp;
new revision: 1.38; previous revision: 1.37
Checking in src/xpwidgets/Makefile.in;
new revision: 1.77; previous revision: 1.76
Checking in src/xpwidgets/nsIdleService.cpp;
initial revision: 1.1
Checking in src/xpwidgets/nsIdleService.h;
initial revision: 1.1

Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 366337
Blocks: 367658
Keywords: dev-doc-needed
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsIdleServiceGTK.cpp#80
 80         PR_LOG(sIdleLog, PR_LOG_WARNING, ("Failed to find libXss.so!\n"));

Might be a good idea to change that to .so.1 as well, so that people don't go looking for the wrong file if they see this message...
Blocks: 380595
This was documented ages ago; marking as such.
Depends on: 397607
When running the following mochitest which tests this feature I get one or more failed passes out of 10.

http://mxr.mozilla.org/seamonkey/source/widget/tests/test_bug343416.xul

"not ok - The idle time should have increased by roughly the amount of time it took for the timeout to fire"

"not ok - We added a listener and it should have been called by now"

How should I proceed? Filing a new bug which depends on that one?
Depends on: 763894
You need to log in before you can comment on or make changes to this bug.