GNOME Bugzilla – Bug 603522
Add ShellDocSystem
Last modified: 2009-12-15 21:17:13 UTC
Some optimizations relating to recent docs; we avoid synchronous stats(), don't recreate all the actors on any change, and defer work until we're mapped.
Created attachment 148853 [details] [review] [ShellDocSystem] Add class for handling recent docs Push some of the JS docInfo down into C; crucially, this lets us use the GIO async API.
Created attachment 148854 [details] [review] Rebase recent documents on top of ShellDocSystem Rather of calling .exists() synchronously for all documents, use ShellDocSystem's async API to only stat docs we're showing. Also, more intelligently do redisplay(); don't recreate actors for recent docs if we already have one for that URI. We may need more intelligent caching here; if e.g. just the associated application changes, we should probably reread the info.
Review of attachment 148853 [details] [review]: Only detailed comments, looks reasonable in broad overview (though we're pretty much there to the point of being able to use GIO async API from JS) ::: src/shell-doc-system.c @@ +36,3 @@ + * @self: A #ShellDocSystem + * + * Returns: (transfer none) (element-type GtkRecentInfo): Cached recent file infos Not sure what "cached" means here - presumably this result is returning some set of GtkRecentInfo that has a coherent purpose and is meaningful to the user? @@ +65,3 @@ + self->priv->idle_emit_deleted_id = 0; + + for (iter = self->priv->deleted_infos; iter; iter = iter->next) Think you should write this reentrant safe - so load the list into a local variable, NULL out self->priv->deleted_infos, then traverse the list @@ +72,3 @@ + * doing the real handling in an idle. + */ + gtk_recent_manager_remove_item (self->priv->manager, gtk_recent_info_get_uri (info), NULL); Do you actually want to do this? What if the recent item is, say, on a flash drive? @@ +106,3 @@ + self->priv->deleted_infos = g_slist_prepend (self->priv->deleted_infos, data->info); + if (self->priv->idle_emit_deleted_id == 0) + self->priv->idle_emit_deleted_id = g_timeout_add_seconds (1, shell_doc_system_idle_emit_deleted, self); method shouldn't be called _idle_ if it's emitted from a timeout, that's just confusing. You are leaking the error @@ +143,3 @@ + file = g_file_new_for_uri (uri); + + g_file_query_info_async (file, "standard::type", G_FILE_QUERY_INFO_NONE, Isn't standard::type enormously expensive if you just want to check for existence? I thought it mime-sniffed in some cases? @@ +152,3 @@ + +/** + * shell_doc_system_check_exists: I think this should be called as something like "queue_existence_check" @@ +154,3 @@ + * shell_doc_system_check_exists: + * @self: A #ShellDocSystem + * @n_items: Count of items to check for existence, starting from most recent How could the caller know how many items to supply here? @@ +176,3 @@ + return; + + self->priv->idle_check_exists_id = g_timeout_add_seconds (2, shell_doc_system_idle_check_exists, self); 2 here seems entirely random and arbitrary; maybe an arbitrary value is appropriate, but it should be encapsulated into a #define and justified as best as possible - why did you pick 2? this is information useful to someone maintaining the code. @@ +190,3 @@ + modified_b = gtk_recent_info_get_modified (info_b); + + return modified_b - modified_a; I think I'd call this function _by_timestamp_descending() or _recently_modified_first() or something. The current name doesn't capture the fact it's a reverse sort, so reading it over the 'modified_b - modified_a' looks wrong. @@ +229,3 @@ + ShellDocSystem *self) +{ + if (self->priv->idle_recent_changed_id > 0) When you mean != 0, say != 0 @@ +231,3 @@ + if (self->priv->idle_recent_changed_id > 0) + return; + self->priv->idle_recent_changed_id = g_timeout_add (0, idle_handle_recent_changed, self); Hmm, again 'idle' here is a bit deceptive even though it's a timeout-0 (which is bit like a high-priority idle). Maybe OK in this usage. I think this serial queuing of stuff to idles and timeeouts is a bit of a bad habit; it can make it pretty hard to tell what is going on and how it will be scheduled. (it took a lot of work to straighten out some problems with idle priorties in libgmenu.) But I don't have a specific recommendation about what ones of these should be removed, so OK until it causes problems. @@ +252,3 @@ + SHELL_TYPE_DOC_SYSTEM, + G_SIGNAL_RUN_LAST, + 0, You use 0 here (as I suggested for changed in the .h file) but not for "changed" ::: src/shell-doc-system.h @@ +22,3 @@ + + ShellDocSystemPrivate *priv; +}; I don't see any reason to use a priv structure, just put the instance and class structures right in the C file... we aren't going to support deriving from this. @@ +28,3 @@ + GObjectClass parent_class; + + void (*changed)(ShellDocSystem *self, gpointer user_data); Missing space after (*changed) - though again not supporting derivation in C, I don't see a reason to use a vfunc here - could just use 0 for the signal offset. @@ +33,3 @@ +GType shell_doc_system_get_type (void) G_GNUC_CONST; + +ShellDocSystem* shell_doc_system_get_default(void); Missing space before (void)
Review of attachment 148854 [details] [review]: Don't particularly like where this ends up with the relationship between DocManager and ShellDocSystem, it doesn't feel like we have a coherent abstraction. Other than that (and various things that look like missing parts of the patch) seems functional. ::: js/misc/docInfo.js @@ +98,1 @@ DocManager.prototype = { Having both DocSystem and DocManager, ugh; the fact that docDisplay.js is mixing the use of the two is very clumsy. The fact we have three levels of changed signals (GtkRecentManager => DocSystem => DocManager) is very clumsy. Etc. Don't have a great recommendation - obviously the docInfo object in JS is useful and it would be a pain to rewrite it in C. But that's sort of the penalty of adding DocSystem... Minimum would be: a) comment for DocManager explaining that the function of DocManager is to translate GtkRecentInfo into DocInfo b) get rid of the use of DocSystem in docDisplay.js and make this a complete wrapper. As a "leaky abstraction" it doesn't work. @@ +119,3 @@ + }, + + getInfosByTimestamp: function() { I don't like here (or in DocSystem) having getInfosByUri return a map from URI to info (which is what you'd expect for a By<X> function) and getInfosByTimestamp return an ordered list. Parallel function names should return similar objects. ::: js/ui/docDisplay.js @@ +153,3 @@ if (!this._docsStale) return true; + this._allItems = this._docManager.getInfosByUri(); It strikes me as confusing to have a member point to a dictionary that is owned by docManager; at a minimum needs a comment pointing that and justifying it (because the dictionary is immutable, if it is, because it's needed for effiency, if it is. Or whatever.) @@ +294,3 @@ + + markDeleted: function() { + this.actor.style_class = 'dash-recent-docs-item-deleted'; Is there a missing CSS addition here? What is the visual effect that happens? Shouldn't the item just be removed? @@ +332,1 @@ + this._actorsByUri = []; I don't think you meant to make this an Array (boo on Javascript that it worked...) @@ +439,1 @@ } I don't think calling this out of allocate() is a good idea @@ +442,3 @@ + for (; i < children.length; i++) + skipPaint.push(children[i]); + this.actor.set_skip_paint(skipPaint); I can't find this in BigBox or in Clutter - did I miss a patch somewhere? @@ +446,3 @@ + _onDestroy: function(actor) { + if (this._idleHandleMappedChangedId > 0) { If you mean != 0, say != 0 @@ +455,3 @@ + let mapped = this.actor.mapped; + if (mapped && this._pendingDocsChange && this._idleHandleMappedChangedId == 0) { + this._idleHandleMappedChangedId = Mainloop.timeout_add(0, Lang.bind(this, this._idleHandleMappedChanged)); This looks superficially good "OK, we'll do an 'idle' at a really high priority, so it has to be called before we actually layout and paint out the actors". But remember that we do all steps of a clutter frame: - event handling - update frames - layout - redraw As a single deterministic process, so there's no return to the main loop between event handling and redraw. So what will happen is that the first frame of the overview coming in will draw with the old contents, then a pause will occur before the next frame and this work will be done. May not be visible, but it's the wrong thing to do. I'm a little skeptical about the whole project of pending work when things are unmapped - it's doing things at the time when we are *most* time constrained - trying to show the overview, but leaving that aside, the scheduling is wrong. What you want is to just do it when the map state changes, there's no possible advantage to pending it. (If there was an advantage, then you'd want meta_later_add(META_LATER_BEFORE_REDRAW) @@ +476,3 @@ + // Should be kept alive by the _actorsByUri + this.actor.remove_all(); + let removedActors = []; Unused @@ +494,3 @@ + if (display.actor.get_parent() == null) { + display.actor.destroy(); + delete this._actorsByUri[uri]; Note for anybody else reviewing - this is OK - ECMA 262 Section 12.6.4 - "Properties of the object being enumerated may be deleted during enumeration."
Created attachment 148952 [details] [review] [ShellGenericContainer] Add set_skip_paint method Ideally we'd be able to override _paint, but given that we can't at the moment, this method gives a way to implement containers which don't happen to paint all of their children.
(In reply to comment #3) > > method shouldn't be called _idle_ if it's emitted from a timeout, that's just > confusing. I use this naming convention in a variety of places; I like having a specific name for functions called from an idle/timeout. Do you have a suggestion for a different name? _async would be even more confusing. _later ? > @@ +143,3 @@ > + file = g_file_new_for_uri (uri); > + > + g_file_query_info_async (file, "standard::type", G_FILE_QUERY_INFO_NONE, > > Isn't standard::type enormously expensive if you just want to check for > existence? I thought it mime-sniffed in some cases? From looking at the source, it looks like on Unix at least we just get the info from stat. _CONTENT_TYPE (not _TYPE) is the one that involves sniffing. > How could the caller know how many items to supply here? This function assumes you're displaying the newest documents; the later shell UI patch knows how many documents it's displaying. > > @@ +176,3 @@ > + return; > + > + self->priv->idle_check_exists_id = g_timeout_add_seconds (2, > shell_doc_system_idle_check_exists, self); > > 2 here seems entirely random and arbitrary; maybe an arbitrary value is > appropriate, but it should be encapsulated into a #define and justified as best > as possible - why did you pick 2? this is information useful to someone > maintaining the code. Yeah, I'll just use a 0 timeout here. > Hmm, again 'idle' here is a bit deceptive even though it's a timeout-0 (which > is bit like a high-priority idle). Maybe OK in this usage. I think this serial > queuing of stuff to idles and timeeouts is a bit of a bad habit; it can make it > pretty hard to tell what is going on and how it will be scheduled. (it took a > lot of work to straighten out some problems with idle priorties in libgmenu.) > But I don't have a specific recommendation about what ones of these should be > removed, so OK until it causes problems. I can probably do the > I don't see any reason to use a priv structure, just put the instance and class > structures right in the C file... we aren't going to support deriving from > this. I find it easier to work on code if I can consistently type ->priv to get at member variables, rather than having to try to remember whether the class uses priv or not.
(In reply to comment #6) > (In reply to comment #3) > > > > method shouldn't be called _idle_ if it's emitted from a timeout, that's just > > confusing. > > I use this naming convention in a variety of places; I like having a specific > name for functions called from an idle/timeout. Do you have a suggestion for a > different name? _async would be even more confusing. _later ? You want a specific name for functions called (non-specifically) from either an idle or timeout? > > @@ +143,3 @@ > > + file = g_file_new_for_uri (uri); > > + > > + g_file_query_info_async (file, "standard::type", G_FILE_QUERY_INFO_NONE, > > > > Isn't standard::type enormously expensive if you just want to check for > > existence? I thought it mime-sniffed in some cases? > > From looking at the source, it looks like on Unix at least we just get the info > from stat. _CONTENT_TYPE (not _TYPE) is the one that involves sniffing. OK, docs seems to correspond. (Any reason you didn't use G_FILE_ATTRIBUTE_STANDARD_TYPE instead of the string?) [...] > > + self->priv->idle_check_exists_id = g_timeout_add_seconds (2, > > shell_doc_system_idle_check_exists, self); > > > > 2 here seems entirely random and arbitrary; maybe an arbitrary value is > > appropriate, but it should be encapsulated into a #define and justified as best > > as possible - why did you pick 2? this is information useful to someone > > maintaining the code. > > Yeah, I'll just use a 0 timeout here. But do you want a 0 timeout? Do we want to animate into the overview first before we start churning IO? > > Hmm, again 'idle' here is a bit deceptive even though it's a timeout-0 (which > > is bit like a high-priority idle). Maybe OK in this usage. I think this serial > > queuing of stuff to idles and timeeouts is a bit of a bad habit; it can make it > > pretty hard to tell what is going on and how it will be scheduled. (it took a > > lot of work to straighten out some problems with idle priorties in libgmenu.) > > But I don't have a specific recommendation about what ones of these should be > > removed, so OK until it causes problems. > > I can probably do the This sentence has no > > I don't see any reason to use a priv structure, just put the instance and class > > structures right in the C file... we aren't going to support deriving from > > this. > > I find it easier to work on code if I can consistently type ->priv to get at > member variables, rather than having to try to remember whether the class uses > priv or not. OK, reasonable opinion, though different from my on the subject.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #3) > > > > > > method shouldn't be called _idle_ if it's emitted from a timeout, that's just > > > confusing. > > > > I use this naming convention in a variety of places; I like having a specific > > name for functions called from an idle/timeout. Do you have a suggestion for a > > different name? _async would be even more confusing. _later ? > > You want a specific name for functions called (non-specifically) from either an > idle or timeout? Right, I tend to think the most salient bit is that it's not called from an event handler basically. > OK, docs seems to correspond. (Any reason you didn't use > G_FILE_ATTRIBUTE_STANDARD_TYPE instead of the string?) Well, it's longer... > But do you want a 0 timeout? Do we want to animate into the overview first > before we start churning IO? Shouldn't block the mutter process at least directly. We could start it only after the animation completes, I don't have a strong opinion here. > This sentence has no haha. Ok, see upcoming deferred work patch.
Created attachment 149452 [details] [review] [main] Add deferred work system Previously we had various things watching for notify::mapped so we could be more lazy about updating non-visible actors, but it was fairly ad-hoc. The deferred work system unifies these callbacks, and also adds a timeout so that we don't delay changes arbitrarily; this way we avoid a storm of work if you stay out of the overview for a while, then go in.
Created attachment 149453 [details] [review] [ShellGenericContainer] Add set_skip_paint method Ideally we'd be able to override _paint, but given that we can't at the moment, this method gives a way to implement containers which don't happen to paint all of their children.
Created attachment 149454 [details] [review] Move Main.currentTime() and Main.createAppLaunchContext() into ShellGlobal Primarily motivated by wanting these functions accessible from C.
Created attachment 149455 [details] [review] [ShellDocSystem] Add class for handling recent docs Push some of the JS docInfo down into C; crucially, this lets us use the GIO async API.
Created attachment 149456 [details] [review] Rebase recent documents on top of ShellDocSystem Rather of calling .exists() synchronously for all documents, use ShellDocSystem's async API to only stat docs we're showing. Use the doc opening functionality in ShellDocSystem. Also, more intelligently do redisplay(); don't recreate actors for recent docs if we already have one for that URI. We may need more intelligent caching here; if e.g. just the associated application changes, we should probably reread the info.
Created attachment 149457 [details] [review] [docDisplay] Limit dash docs to 50 We has poor performance if we try to allocate hundreds of actors; a bit more ideally, we'd keep pulling as much as we can, but really no one should have more than 50 in the list as is now except on unreasonably large screen sizes. In the future we should change this to be pull-on-demand thing in a chunked fashion.
Review of attachment 149452 [details] [review]: I like how this worked out in appDisplay.js - not only does it seem like a more robust way of doing things and less likely to cause problems with stalling the overview getting mapped, it significantly reduces the amount of code. Lots of comments on details, but the big picture looks good. API looks reasonable, thought maybe a little bit "non-JS-y"; also seems that "map" should be mentioned somewhere in the API, since the whole operation is really about that. I guess another API approach would have been: this._glowRerenderer = new Main.DeferToMap(this.actor, Lang.bind(this, this._rerenderGlow)); [...] this._glowRerenderer.queueWork(); Or even: this._rerenderGlow = Main.deferToMap(this.actor, Lang.bind(this, this._rerenderGlow)); [...] this._rerenderGlow(); (So, wrapping the prototype method with an identically named instance method. Crack?) I'm OK with the current approach too, we can always refine later if we want. ::: js/ui/appDisplay.js @@ +961,2 @@ + AppFavorites.getAppFavorites().connect('changed', Lang.bind(this, this._queueRedisplay)); + this._tracker.connect('app-running-changed', Lang.bind(this, this._queueRedisplay)); You have three parallel statements as <line><blank line><line><line> ::: js/ui/main.js @@ +427,3 @@ } + +const defaultDeferredTimeoutSeconds = 20; Don't we use DEAFULT_DEFERRED_TIMEOUT_SECONDS for constants? @@ +429,3 @@ +const defaultDeferredTimeoutSeconds = 20; +const _deferredWorkData = {}; +const _deferredWorkQueue = []; It's strange to me to have these as const - I guess it works fine unless you want to whole-sale replace the queue, but it isn't something we do elsewhere. Thought question - the only time that you actually maintain order as queued is when things get run at idle 20 seconds later - you don't do that if things get mapped in the mean time. So is there any reason to use an array, or could you use a {} object for _deferredWorkQueue? Would slightly simplify the code and avoid some O(n**2) splicing, not that probably is detectable on profiling even if 50-100 things end up in the work queue. If order is actually important, then it probably is a good idea to use a single BEFORE_REDRAW and preserve order there as well. @@ +430,3 @@ +const _deferredWorkData = {}; +const _deferredWorkQueue = []; +// Integer id used as a key into _deferredWorkTypes Comment refers to a variable that (no longer?) exists @@ +438,3 @@ + if (index < 0) { + return; + } Excess {} @@ +450,3 @@ + while (_deferredWorkQueue.length > 0) { + _runDeferredWork(_deferredWorkQueue[0]); + } Excess {} @@ +451,3 @@ + _runDeferredWork(_deferredWorkQueue[0]); + } + return false; You don't (no longer?) use this as a idle/timeout, so it doesn't need a return value @@ +457,3 @@ + * @workId: A string identifier for the deferred work + * @actor: A #ClutterActor + * @timeoutSeconds: Timeout in seconds, or -1 for default You didn't implement it. (If you don't implement it, then defaultDeferredTimeoutSeconds probably should lose the 'default') @@ +469,3 @@ + * However, ensure the function is called after a timeout if the actor + * hasn't been mapped in that time, to avoid excessive redraw stalls when + * a lot of actors suddenly become mapped (again, like the overview). Not clear here who is does the "ensure" or whether you are saying that the caller needs to do that. "The reason we do the work after a timeout if the actor hasn't been mapped is..." @@ +475,3 @@ +function initializeDeferredWork(actor, callback) { + // Turn into a string so we can use as an object property + let workId = "" + (_deferredWorkSequence++); I think it's better to start off with 1... will be obvious in debugging if you see 1 that it isn't a special flag value. ("0" is true, so that concern isn't the same as for 0, but still, I think "1" is just clearer.) So ++_deferredWorkSequence rather than the reverse. @@ +480,3 @@ + actor.connect('notify::mapped', function () { + if (actor.mapped && _deferredWorkQueue.indexOf(workId) >= 0) { + Meta.later_add(Meta.LaterType.BEFORE_REDRAW, function () { _runDeferredWork(workId); return false; }, null); once the function has more than one statement, I think it's pretty clumsy and hard to read to have it on one line like this. You have a bug here and below if the actor is destroyed between this point and when _runDeferredWork(workId) is called - maybe just check that _deferredWorkData[workId] exists in runDeferredWork(). @@ +487,3 @@ + if (index >= 0) { + _deferredWorkQueue.splice(index, 1); + } Excess {} @@ +490,3 @@ + delete _deferredWorkData[workId]; + }); + queueDeferredWork(workId); This needs to be documented fairly prominently in the docs. I'm guessing that the reasons for it are: - It is basically universal to do "deferred work" on actor creation - The actor might not be mapped initially on creation My main reservations with this are a) it's a little unexpected b) if you avoid some portion of initialization and do it BEFORE_REDRAW, then the actor is in an uninitialized state between creation and that BEFORE_REDRAW cycles, which could lead to very hard to diagnose problems when additional events are processed before that BEFORE_REDRAW cycle. Not hard-and-fast opposed, but you probably need a BOLD note in the docs pointing out that trap if you want to go this way. @@ +501,3 @@ + * run on map or timeout. You should call this function + * for example when data being displayed by the actor has + * changed. You should document what happens when the actor is already mapped. @@ +512,3 @@ + _deferredWorkQueue.push(workId); + if (data.actor.mapped) { + Mainloop.timeout_add(0, function () { _runDeferredWork(workId); return false; }); Anything you do at a 0 second timeout is done *one frame later*. This is asking for odd visual artifacts, plus extra drawing and slower performance. Just as you do for notify::mapped, you should use: Meta.later_add(Meta.LATER_BEFORE_REDRAW, function() {}); The only time it won't work is if this is already being called from a BEFORE_REDRAW callback. Hmm, that's actually a somewhat nasty trap - it means that things won't work as expected if you use this functionality to create actors that themselves use this functionality. That might be another reason to use a single BEFORE_REDRAW rather than a bunch of them - then you can make sure that all work on mapped actors is done before you return and let clutter continue on to allocate and redraw. (You might ask if that actually should be pushed up to Meta.later_add() - should *that* make sure that BEFORE_REDRAW laters that are added from another BEFORE_REDRAW later are run immediately? Possible, but I was worried about infinite 100% cpu loops in that case, since it's a very generic functionality. Here we are somewhat more constrained about what we are doing, so it seems like a better place to do it.)
Review of attachment 149453 [details] [review]: Looks OK, but I really don't like the API. My suggestion below is definitely not as efficient (expecially prior to your invocation speedups), but unless profiling shows a bottleneck, using a "weird" API strikes me as premature micro-optimization. ::: src/shell-generic-container.c @@ +211,3 @@ + GList *iter, *children; + + (CLUTTER_ACTOR_CLASS (g_type_class_peek (clutter_actor_get_type ())))->pick (actor, color); The extra parens here are non-standard. ::: src/shell-generic-container.h @@ +44,3 @@ +void shell_generic_container_set_skip_paint (ShellGenericContainer *container, + GSList *actors); I think this should be void shell_generic_container_set_skip_paint (ShellGenericContainer *container, ClutterActor child, gboolean skip_paint);
Review of attachment 149454 [details] [review]: Looks fine to commit after fixing a couple of tiny style comments ::: src/shell-global.c @@ +1067,3 @@ + +/** + * shell_global_get_current_time: Would prefer if this was valid gtk-doc with @global: a #ShellGlobal and a Return value: @@ +1080,3 @@ + MetaDisplay *display; + + // meta_display_get_current_time() will return the correct time No C++/C99 comments in C code, some compilers will choke. @@ +1107,3 @@ + * targeted to activate on the current workspace. + * + * Returns: (transfer none): A new floating #GAppLaunchContext It's annoying that gtk-doc accepts either, but I've been trying to standardize on Return value: not Returns: for our code, because it seemed we were doing it that way in more places. @@ +1121,3 @@ + gdk_app_launch_context_set_desktop (context, meta_screen_get_active_workspace_index (shell_global_get_screen (global))); + + return (GAppLaunchContext*)context; 'GAppLaunchContext *'
Review of attachment 149454 [details] [review]: ::: src/shell-global.c @@ +1107,3 @@ + * targeted to activate on the current workspace. + * + * Returns: (transfer none): A new floating #GAppLaunchContext After further research, I don't think the returned GAppLaunchContext is floating, this should be (transfer full)
Review of attachment 149455 [details] [review]: Generally looks good, few details need fixing. Might be nice to have a brief section doc comment that gives a high-level description. ::: src/shell-doc-system.c @@ +35,3 @@ + * Returns all recent files. This function does not perform I/O. + * + * Returns: (transfer none) (element-type GtkRecentInfo): Cached recent file infos Better than last time, but I still can't really connected "Cached recent file infos" with a meaning just from reading this. And if the function doesn't perform I/O, how does it find your recent files? it would be useful to go into a little more detail here and say something like "Returns the last set of recent files. Recent files are read asynchronousl, so when called on a newly created #ShellDocSystem, returns an empty list". Or something like that. Then "cached" starts to make sense. @@ +230,3 @@ + GAppInfo *app_info; + gboolean needs_uri; + GList *uris; Should move this into the inner block since it's uesd only in one branch. Also, you leak the list. @@ +253,3 @@ + { + GRegex *regex; + // TODO: Change this once better support for creating C++/C99 comments in C code @@ +257,3 @@ + // this relies on the fact that the file uri is + // already a part of appExec, so we don't supply any + // files to appInfo.launch(). app_info.launch() or g_app_info_launch() @@ +279,3 @@ + // despite passing the app launch context, no startup + // notification occurs. + g_app_info_launch (app_info, NULL, shell_global_create_app_launch_context (shell_global_get ()), NULL); You documented shell_global_create_app_launch_context() as returning a floating app launch context, but I can find no evidence that GdkAppLaunchContext derives from GInitiallyUnknowned or that g_app_info_launch() sinks the result - I think you are just leaking.
Review of attachment 149455 [details] [review]: Followup question on ShellDocSystem - isn't it a problem that the signal for: - The list of recent documents changing - Files being deleted Are both ::changed if you are expected to call queue_existence_check() when you get ::changed? Isn't that going to mean that if I have some deleted file in my recent documents, every time the list of recent documents changes, I'll stat() all the files *twice*? If I'm missing something about that works, it might be good to make sure the section comment has enough detail about expected usage to make that clear. (Also wonder if there should be generally be some sort of minimum-stat-interval for each file, so if stuff gets added to the recent files list gradually we aren't continually stat'ting things over and over; well, that probably would be a lot of extra complexity, and if it's interesting, a separate bug.)
Review of attachment 149456 [details] [review]: Generally looks good, just one comment. ::: js/ui/docDisplay.js @@ +431,3 @@ + if (this._checkDocExistence) { + this._docManager.queueExistenceCheck(i); + this._checkDocExistence = false; As commented previously, I *really* don't like doing this out of allocate(); the allocate() function should allocate, it shouldn't queue random work for later. If there's some overriding reason to do it this way, document it, otherwise find a different place to do it. If you want to avoid the IO and CPU usage from the checks interfering with the animation into the overview, then you should do it *after* the overview finishes animating in. If you don't care about that, you might as well do it right out of _redisplay(); I don't see the point of doing it a frame or so into the animation.
Review of attachment 149457 [details] [review]: I have 38 actors on my list on 1400x1050. Not at home now so I can't check, but I bet at 1920x1200 we cram in more than 50. Still, it probably looks fine to just have 50 - that's already quite a lot and pretty hard to find anything in there visually.
Review of attachment 149456 [details] [review]: ::: js/ui/docDisplay.js @@ +431,3 @@ + if (this._checkDocExistence) { + this._docManager.queueExistenceCheck(i); + this._checkDocExistence = false; Ah, light went on - you are doing it here because know you know how many you are displaying. Something like: // Now we know how many docs we are displaying, queue a check to see if any of them // have been deleted. If they are deleted, then we'll get a 'changed' signal; since // we'll now be displaying items we weren't previously, we'll check again to see // if they were deleted, and so forth and so on. I'm not very happy about this, really. I think it's not too uncommon to have 150 recent documents from a thumbdrive that is no longer plugged in in your recent documents list, you shouldn't see them scroll page by page through your recent documents when you first go to the overview. (And if you have a small screen, then that "page by page" might be 10 at a time.) Not a blocker for landing this, but I think we will need to figure out something better. (One fairly simple possibility is to have queueExistenceCheck(count) mean "check for existence until you find count non-deleted items"; which would limit things to one redisplay, not an arbitrary many.)
(In reply to comment #20) > Review of attachment 149455 [details] [review]: > > Followup question on ShellDocSystem - isn't it a problem that the signal for: > > - The list of recent documents changing > - Files being deleted > > Are both ::changed if you are expected to call queue_existence_check() when you > get ::changed? Isn't that going to mean that if I have some deleted file in my > recent documents, every time the list of recent documents changes, I'll stat() > all the files *twice*? When ShellDocSystem notices something has been deleted, it's removed from self->priv->infos_by_timestamp, which is what queue_existence_check uses. So deleted won't be re-emitted, but yes we will eventually stat the visible docs every time the list changes (but with deferred work, this is fine outside of the overview). > (Also wonder if there should be generally be some sort of minimum-stat-interval > for each file, so if stuff gets added to the recent files list gradually we > aren't continually stat'ting things over and over; well, that probably would be > a lot of extra complexity, and if it's interesting, a separate bug.) Yeah...clearly a lot more possibilities here, e.g. using inotify for a subset of them, etc etc. But I'd like to move from totally-broken to could-be-better before getting to good =)
(In reply to comment #19) > Review of attachment 149455 [details] [review]: > > Generally looks good, few details need fixing. Might be nice to have a brief > section doc comment that gives a high-level description. > > ::: src/shell-doc-system.c > @@ +35,3 @@ > + * Returns all recent files. This function does not perform I/O. > + * > + * Returns: (transfer none) (element-type GtkRecentInfo): Cached recent file > infos > > Better than last time, but I still can't really connected "Cached recent file > infos" with a meaning just from reading this. And if the function doesn't > perform I/O, how does it find your recent files? it would be useful to go into > a little more detail here and say something like "Returns the last set of > recent files. Recent files are read asynchronousl, so when called on a newly > created #ShellDocSystem, returns an empty list". Or something like that. Then > "cached" starts to make sense. Well, it's not read asynchronously unfortunately; that would require GtkRecentManager surgery. I updated the comment though.
(In reply to comment #23) > > I'm not very happy about this, really. I think it's not too uncommon to have > 150 recent documents from a thumbdrive that is no longer plugged in in your > recent documents list, you shouldn't see them scroll page by page through your > recent documents when you first go to the overview. (And if you have a small > screen, then that "page by page" might be 10 at a time.) Not a blocker for > landing this, but I think we will need to figure out something better. (One > fairly simple possibility is to have queueExistenceCheck(count) mean "check for > existence until you find count non-deleted items"; which would limit things to > one redisplay, not an arbitrary many.) This is a good idea, I added a TODO.
(In reply to comment #15) > Review of attachment 149452 [details] [review]: > > I like how this worked out in appDisplay.js - not only does it seem like a more > robust way of doing things and less likely to cause problems with stalling the > overview getting mapped, it significantly reduces the amount of code. > > Lots of comments on details, but the big picture looks good. > > API looks reasonable, thought maybe a little bit "non-JS-y"; also seems that > "map" should be mentioned somewhere in the API, since the whole operation is > really about that. > > I guess another API approach would have been: Punted on this for nwo. > Thought question - the only time that you actually maintain order as queued is > when things get run at idle 20 seconds later - you don't do that if things get > mapped in the mean time. So is there any reason to use an array, or could you > use a {} object for _deferredWorkQueue? Would slightly simplify the code and > avoid some O(n**2) splicing, not that probably is detectable on profiling even > if 50-100 things end up in the work queue. Originally because the work ids were integers, and I still think of it mainly as a queue, where things being pulled out on demand is an exception and not the rule. The other thing is when i briefly started trying to switch I wasn't sure offhand how to change "if _deferredWorkQueue.length > 0 { schedule timeout }" without another variable, and that made it less compelling. > If order is actually important, then it probably is a good idea to use a single > BEFORE_REDRAW and preserve order there as well. Order shouldn't be important no. > That might be another reason to use a single BEFORE_REDRAW rather than a bunch > of them - then you can make sure that all work on mapped actors is done before > you return and let clutter continue on to allocate and redraw. I used a single BEFORE_REDRAW now.
Attachment 149454 [details] pushed as 42757a0 - Move Main.currentTime() and Main.createAppLaunchContext() into ShellGlobal Attachment 149456 [details] pushed as 949f674 - Rebase recent documents on top of ShellDocSystem