In __set_visible_status the first statement needs to be
self.visible_status_id = 0 otherwise subsequent getting descriptions
does not work properly.
In __do_search self.__setup_before_single_search has moved out of if
statment and changed into self.__clear_before_search()
What is the reason for this?
A nit:
Line 1660: Four spaces too many
Padraig
On 06/29/09 12:14, jmr wrote:
Thanks Padraig - made the changes, tested them and working fine, new
webrev, which applies cleanly against the gate.
http://cr.opensolaris.org/~jmr/pm_8324_local_search_using_api_29Jun_1210pm/
JR
Padraig O'Briain wrote:
John,
Just a couple of comments:
1) I think that __set_visible_status should not do anything if the
start page is showing. I suggest adding the following at line 1317
if self.w_main_view_notebook.get_current_page() != \
NOTEBOOK_PACKAGE_LIST_PAGE:
return
Otherwise descriptions are retrieved twice if category is clicked
immediately on startup.
2) Consider the following:
a) Start package manager
b) Switch to Search All Repositories
c) Switch back to Search Current Repository
d) Click on Accessories
Nothing happens!
I have no idea what is going wrong here. It looks like the filtering
is working as expected but the application list does not change.
I think the problem here is the following lines need to be added to
__save_setup_before_search:
self.saved_application_list_filter = \
self.application_list_filter
Padraig
Padraig
jmr wrote:
Thanks Brock, much appreciate the feedback. See comments below:
New webrev with your, Padraig's and Shawn's changes:
http://cr.opensolaris.org/~jmr/pm_8324_local_search_using_api_26Jun_5pm/
JR
Brock Pytlik wrote:
jmr wrote:
I see Padraig's response on the icon issue which might be at the
root of your problems.
I have just applied the webrev against the latest gate, it had
some hunk offsets but was still ok, then did a full make install
and make packages. I have sorted Padraig's issue on the Mange
Repositories Reload as he had suggested and generated a new
webrev against the latest gate.
http://cr.opensolaris.org/~jmr/pm_8324_local_search_using_api_25Jun_4pm/
packagemanager.py:
271: since this is only used to track search's elapsed, time,
maybe the variable should include search in its name.
Sure
As a bigger question, why are we tracking and reporting the search
time to the user? From a UI perspective, I'm not sure why a user
would care whether search took 7 seconds or 9 seconds. Is there a
reason a user would want this info or is it more a
debugging/development tool for you?
Well in Google the search time is reported in brackets at the top
of the page, but its just a nice to have. At present I wanted it
there to help with the debugging and optimization work for Search.
Happy to change it or take it out given feedback from users.
I'm slightly confused about what the "current_not_show_repos"
variable means. On lines 807-810 it seems like those are exactly
the ones you're showing the user as having errors. Also, when does
the variable get cleared? If, and here I'm just guessing, it's a
list of repos not to search, does one failed search mean you won't
retry that publisher again until the next time packagemanger is
restarted?
I agree "current_not_show_repos" is misleading, renamed to
"current_repos_with_search_errors", which captures the intent here.
It was being unset in a number of places redundantly, it is now
just unset at the start of __parse_api_search_error() where it is
populated and then used in __handle_api_search_error.
When errors occur searching a repo the user is notified by a dialog
and a yellow warning triangle in the status bar:
__do_api_search
:
except api_errors.ProblematicSearchServers, ex:
self.__parse_api_search_error(ex)
gobject.idle_add(self.w_progress_dialog.hide)
gobject.idle_add(self.__handle_api_search_error)
The user can elect to not have these errors reported again in the
popup dialog. These repos to ignore on error are stored and are
persistent across invocations of PM. It does not mean that you
won't try and search these repos, only that the user won't be
bugged with errors telling them there are problems with it. What
happens in this case is that a yellow triangle is all they see in
the statusbar if there are errors on a given repo they have chosen
to ignore. Clicking on the triangle brings up the dialog listing
all the repos with errors and unsets the ignore state of these repos.
For lines 780-782, why are you getting the string of the error and
testing the string instead of looking at the class of the
exception using, for example, isinstance? That would give you far
more control over the information you display to the user as well
as allowing you to get the specific url, error code, message
etc... Also, it would make more explicit which errors you've
accounted for and which are getting ignored. For example, if I've
followed the code correctly, if search encountered a BadStatusLine
error for a repo, the user would never be notified of this
problem, and would simply see that their search returned no results.
I am leaving this until J's stuff lands and will rework, given his
and your recent proposal on:
http://defect.opensolaris.org/bz/show_bug.cgi?id=9670#c2
I realize that a lot of this code will need to get rewhacked when
J's transport wad lands, but explicitly checking for specific
errors and handling them appropriately seems much closer to what
the code will look like in that world than the current approach.
1736: Why do case sensitive search by default? At least for
command line search, all the feedback we've gotten suggests that
case insensitive should be the default. Typically, if someone
wants to search for openoffice, they don't want to have to
remember the exact capitalization pattern used.
I thought that's what we were doing:
[api.Query(" ".join(pargs), False, True, None,
None)], servers=servers
query_p.Query.__init__(self, text, case_sensitive, return_type,
num_to_return, start_point)
Searching for OFFice and office turn up 26 hits in opensolaris.org
1740-1750 or so: I realize this hasn't changed in this fix,
I'm a bit confused about the approach taken here. Why create an
artificial limit on the number of results generated, why not treat
this as a generator and have it throw results to the user as soon
as they're ready? When we switched to that model on the cli, the
user experience was vastly improved because results started
appearing much sooner. Also, if you're going to limit the number
of results shown to the user to be 100, why not tell the servers
not to return more than 100 results?
Good point - I like the idea of getting the results back
incrementally. I have submitted this as an RFE and will work on it
after this wad goes back.
9710 PM search should return results incrementally
1775, 1781: Why do insert 0, which is slow for long lists, and the
reverse the results? Can't this just be results.append(a_res), and
then you don't need to do the reverse either?
Done
Nits:
239: commented line
Done
266: extra whitespace
Done
1673: why not just pass self.is_search_all here instead of
creating search_all?
Done
Thanks,
Brock
JR
Shawn Walker wrote:
On Jun 25, 2009, at 6:04 AM, jmr wrote:
Hi , here is a webrev to move PM over to using the Search API
for all searches, both against single publishers (previously
used GtkTreeView filtering) and all publishers (added in
2009.06, already using Search API).
webrev:
http://cr.opensolaris.org/~jmr/pm_8324_local_search_using_api_25Jun_1150am/
9442 Use Search API in PM for all searches
This will cause a slow down when searching against Single
Publishers. The timings for a search are listed in the Status
Bar if it takes longer than 1 sec. If a repo cannot be
contacted there is a time out of 30 secs, which is caught and
reported as a failed search against the repo. When we have
better exceptions here from the transport layer [9670] we can
do a better job of handling the range of errors.
Any idea why I see this when trying to start packagemanager from
my workspace?
Traceback (most recent call last):
File "./packagemanager.py", line 4032, in ?
packagemanager = PackageManager()
File "./packagemanager.py", line 265, in __init__
self.__register_iconsets(self.search_options)
File "./packagemanager.py", line 644, in __register_iconsets
iconset = gtk.IconSet(pixbuf) TypeError: pixbuf should be a
GdkPixbuf
I've blown away my proto area and done a make install, and I've
checked that my PACKAGE_MANAGER_ROOT is set right.
I'm on build 111b here.
Cheers,
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
