43.69 KB, image/png
57.87 KB, image/png
43.93 KB, image/png
1.10 KB, patch
|Details | Diff | Splinter Review|
49 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
Right now you have to enter the date manually (i.e. 2015-11-24) in the field when starting a bisection. It would be nice if we could use some kind of datepicker widget to do this instead. Something like this: http://jqueryui.com/datepicker/ (obviously you'd be using the Qt equivalent, that's for the web)
I think we can use QCalendarWidget (http://pyqt.sourceforge.net/Docs/PyQt4/qcalendarwidget.html) for this. Thoughts, @parkouss & @wlach?
Created attachment 8700566 [details] date-picker.png This is how it looks with my current changes. Should I try to make it exactly like http://jqueryui.com/datepicker/. I meant, having a text box and when we click on it, calendar pops up ?
Oh, I was thinking about yet another dialog (in addition to the textbox) - but this looks quite cool to me! Will, what do you think ? Look at the attachment above.
Yeah, this looks good to me! I'd put the dropdown selecting type beside "Last known good" and "First known bad" though. Text nits: 1. Change 'Bisection range selection' to just 'Range selection' 2. This is probably a longstanding issue, but I'd change 'Search fix' to 'Search for fix'.
Looks pretty good to me, thanks! I have to test it on windows to see how it looks like though, since most of our users are on windows. I'll do that once you are happy with the code and ask for a review.
Created attachment 8700867 [details] date-picker-2.png Aligned the comboboxes with "First known bad build" and "Last known good build" labels.
Created attachment 8701105 [details] [review] Pull Request
Comment on attachment 8701105 [details] [review] Pull Request So, this looks very good - this is really an improvement for the date selection! I love this calendar widget. Though there is one problem, if I change the selection method from date to changeset for example, the QLineEdit used for the changeset takes the same place as the calendar - and this is quite horrible. :( I believe this can be fixed quite easily, quick search on internet: http://stackoverflow.com/questions/14480696/resize-qstackedwidget-to-the-page-which-is-opened#14485901 Tell me if you also have the same behavior (I'm running Linux/Gnome3) - then please fix this and ask again for a review. Once fixed it will be awesome!
Yes, I am also noticing the same behavior on OSX. Terribly sorry for not checking this properly. Will fix it.
No problem! Thanks for looking at it. :)
Status: NEW → ASSIGNED
Created attachment 8701704 [details] date-picker-3.png I have been able to fix it but not completely. What is happening is that when you select 'release' for the first time, it displays that huge combobox. But, if you select 'release', then some other option, say 'build' and then again 'release', it displays the nice small combobox. I tried multiple things like subclassing QStackedWidget and overriding sizeHint() and minimumSizeHint(). I also tried to play around with setSizePolicy() and the adjustSize() methods. But this is one place where I am stuck. Any suggestions?
Hmm, hard to tell without seeing any code - but it looks like maybe the signal slot connection to resize the currentChange widget was done at a wrong time (since it seems to work for every but the first time). Try debugging it - maybe some print statements to be sure it is really doing what you think - then show us some code if you're stuck. :)
Created attachment 8701817 [details] [review] Pull Request
Comment on attachment 8701817 [details] [review] Pull Request Unfortunately, if you choose changeset, then go back to date, the widget uses a minimum size... Also resizing the dialog break everything again. :( I quickly tried this: http://stackoverflow.com/a/14486499 This works, but is a bit brutal as it move every other widget along. But this is acceptable - take a look at it if you want, or try something else! Thanks for sticking with it. :)
I am confused as I am not seeing the behavior you mention. When I change the from changeset to date, it works fine for me. Also, I don't understand the resizing the dialog part. Should everything scale to a proper size as the dialog is resized?
Created attachment 8701828 [details] date-changeset-date.png this is what I have if I select changeset, then date again.
Created attachment 8701829 [details] after-resizing.png and here, I choose changeset, the resize the dialog.
(In reply to Julien Pagès (:parkouss) from comment #19) > Created attachment 8701829 [details] > after-resizing.png > > and here, I choose changeset, the resize the dialog. I mean I choose release - but same thing I guess.
I am also seeing the issue you mentioned on Ubuntu. However, I tried both the accepted solution and the solution you mentioned on the linked SO question. None of them seemed to work for me. Any ideas?
(In reply to Saurabh Singhal from comment #21) > I am also seeing the issue you mentioned on Ubuntu. However, I tried both > the accepted solution and the solution you mentioned on the linked SO > question. None of them seemed to work for me. Any ideas? Hm, the solution on SO worked for me - it has to be done in the slot each time the widget changes. I can show you the code if you need, tell me.
Created attachment 8702066 [details] [diff] [review] datepicker.patch So, here's the code I tried. The approaches: 1. Uncommented code: the accepted solution on SO. 2. Commented code: the solution you mentioned. Also, for this code, I used self.stacked.currentChanged.connect(self.onCurrentChanged). So, what needs to be changed here?
Created attachment 8702110 [details] [diff] [review] working.diff There is a couple of issues in your implementation from SO. Here is a working patch for me, tell me if that works.
Created attachment 8702111 [details] datepickerUbuntu.png I'm sorry but this doesn't work for me. When we select date->some other field->date, the calendar almost disappears.
(In reply to Saurabh Singhal from comment #25) > Created attachment 8702111 [details] > datepickerUbuntu.png > > I'm sorry but this doesn't work for me. When we select date->some other > field->date, the calendar almost disappears. No problem! Well, I don't see this. I applied this patch on top of your branch crisron:date-picker (no other modification) and it works fine. Though, I'm using gnome 3.18.3 on archlinux, maybe it is different from your system/gnome version and could explain the difference. A last idea that comes to mind would be to put in the stacked widget some wrapper widget for the QlineEdit fields, that would use vertical spacers to force the widget to not be expendable. A good thing would be to create a new .ui file to create the interface of RangeSelection, then adapt the RangeSelection class to use this new .ui file. Does that make sense to you ?
Being a beginner in PyQt, I don't understand much of what you are proposing. Some questions: 1. What wrapper widget would I use? How? 2. In the new .ui file, should I create all the widgets I use in RangeSelection? Would I need to create a python file for this too?
Oh, I guess the questions here have been answered on irc. Saurabh, are you still working on this bug ?
Yes. There has been a lot of office work for the past 2 weeks. So, I was not able to give this bug much time. I plan to finish it up this weekend.
Created attachment 8713432 [details] [review] Pull Request
Attachment #8700566 - Attachment is obsolete: true
Attachment #8700682 - Attachment is obsolete: true
Attachment #8700867 - Attachment is obsolete: true
Attachment #8701105 - Attachment is obsolete: true
Attachment #8701704 - Attachment is obsolete: true
Attachment #8701817 - Attachment is obsolete: true
Attachment #8702066 - Attachment is obsolete: true
Attachment #8702111 - Attachment is obsolete: true
Attachment #8713432 - Flags: review?(j.parkouss)
Comment on attachment 8713432 [details] [review] Pull Request Great stuff. And thanks a lot for all the renaming of range to build! I just noted a small nit in the PR, could you fix it please ? this is about the use of self.helperUI. Once fixed, I'll be happy to merge this! Just ping me when you're done - thanks!
Attachment #8713432 - Flags: review?(j.parkouss) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.