GNOME Bugzilla – Bug 642237
Clean up history management between runDialog and lookingGlass.
Last modified: 2011-02-19 19:27:03 UTC
The runDialog and the lookingGlass both implement a home-made history manager, each working differently than the other. These patches simplify history management as in readline by adding a general history manager supporting reading and writing to gsettings, and takes out each history manager to use this. In the future, a stock "HistoryEnabledEntry" component may be useful.
Created attachment 180779 [details] [review] Add a common manager for saving and reading history from GSettings, to replace the individual managers in the panel and lookingGlass itself
Created attachment 180780 [details] [review] runDialog: use new history manager implementation
Created attachment 180781 [details] [review] lookingGlass: use new history manager implementation This starts saving lookingGlass history in gsettings, and also adds the ability to clear the text field by pressing 'down' on the last entry, like the run dialog and readline allow.
Review of attachment 180779 [details] [review]: Overall looks good, some comments bellow though. And use a shorter subject line and add the details in the commit message itself. ::: js/Makefile.am @@ +11,3 @@ misc/telepathy.js \ misc/util.js \ + misc/history.js \ This list is supposed to be ordered alphabetically. ::: js/misc/history.js @@ +26,3 @@ + }, + + up: function(text) { I do not like the names "up" and "down" here they do represent the key used to access them in text entries but have no meaning out of this context. I'd rather use "prev/next" or even "prevItem" and "nextItem". @@ +41,3 @@ + }, + + pushInput: function(input) { Naming nitpick again ;) I would call this "addItem". @@ +64,3 @@ + }, + + save: function() { Why is this public? You only access it from pushInput and I do not see a point in calling save() without actually adding new items.
Review of attachment 180780 [details] [review]: Looks good, will need adjustments once you fix up the previous patch though.
Review of attachment 180781 [details] [review]: Looks good, will need rebasing too.
Created attachment 181349 [details] [review] Add a history manager runDialog and lookingGlass both implement a home-made history manager, each working slightly differently than each other in behavior and implementation. Extract the behavior and implementation from runDialog, which reads and saves to GSettings.
Created attachment 181350 [details] [review] Add a history manager runDialog and lookingGlass both implement a home-made history manager, each working slightly differently than each other in behavior and implementation. Extract the behavior and implementation from runDialog, which reads and saves to GSettings.
Created attachment 181352 [details] [review] Add a history manager runDialog and lookingGlass both implement a home-made history manager, each working slightly differently than each other in behavior and implementation. Extract the behavior and implementation from runDialog, which reads and saves to GSettings. sorry about bugspam, was trying to wrangle git-bz under control
Created attachment 181353 [details] [review] runDialog: use new history manager implementation
Created attachment 181354 [details] [review] lookingGlass: use new history manager implementation This starts saving lookingGlass history in gsettings, and also adds the ability to clear the text field by pressing 'down' on the last entry, like the run dialog and readline allow.
Review of attachment 181352 [details] [review]: Looks good.
Review of attachment 181353 [details] [review]: This one too.
Review of attachment 181354 [details] [review]: And this.
Attachment 181352 [details] pushed as cf3c631 - Add a history manager Attachment 181353 [details] pushed as 30da70a - runDialog: use new history manager implementation Attachment 181354 [details] pushed as 4bedbca - lookingGlass: use new history manager implementation