1235003 - [gui] set last good/bad selection as default for every verdict

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[YF (Yang)]

I feel should assume that users don't needs to change the last option (continue to looking) to reduce an unnecessary choice. It fit the user interface design and psychology, there is no needs to change, so I chose the Next button.

Comment 1

[Julien Pagès (:parkouss)]

Oh, you mean we should "continue to bisect further" automatically, without asking any question ?

If that is your proposal, I agree with that. :) This is automatically what a user would want to do, you are right - and so currently a quite useless and annoying dialog.

Thanks Yang for the bug reports by the way!

Comment 2

[YF (Yang)]

Uh, no, that is another ask, but I also agree that.

I just hope the Build verdict auto selected last option, if it as good or bad, because the same situation with last time is more common and bored.

Comment 3

[Julien Pagès (:parkouss)]

Hum, we removed the dialog for the build verdict, it is now integrated in the bisection view panel (not yet released). Though we can still provide by default the last selected choice.

Comment 4

[YF (Yang)]

I'm not sure whether the idea is useful, it is just an idea, and it can be used as an optional option if you are willing to do.


In addition, the current interface is good, even though it addition one-click operation for for non-good, i.e. click the list, select, click the button. The can be improved to two buttons (good and bad) and a dropdown list (others/actions) to reduce one operation for bad.

Comment 5

[Julien Pagès (:parkouss)]

(In reply to YF (Yang) from comment #4)
> In addition, the current interface is good, even though it addition
> one-click operation for for non-good, i.e. click the list, select, click the
> button. The can be improved to two buttons (good and bad) and a dropdown
> list (others/actions) to reduce one operation for bad.

Oh, I like this idea. :)

Comment 6

[Wasif Hyder]

Hi Julien.

I'd like to get started with this bug. 

:)

Comment 7

[Julien Pagès (:parkouss)]

Sure! So I think we should try to display two buttons, one for good verdict, one for bad verdict, and a dropdown list for other choices (instead of the current list with one evaluate button). Tell me if you want me to help you getting started with more information.

Comment 8

[Wasif Hyder]

Yeah, I'd prefer a bit more information since I feel lost currently. My understanding is that you want to add a feature for bisections, but beyond that I'm a little new.

Comment 9

[Julien Pagès (:parkouss)]

(In reply to wasif_hyder from comment #8)
> Yeah, I'd prefer a bit more information since I feel lost currently. My
> understanding is that you want to add a feature for bisections, but beyond
> that I'm a little new.

Not exactly - if you start a bisection (with all default parameters), it will download a build, run it. At this point there will be a row in the bisection view with a dropdown list, and an evaluate button. What we want to do is to to replace that with two buttons, and one dropdown list, as explained in comment 7. So this is just a change of the interface: we want to remove the states "good" and "bad" from the current drop down list - they should be replaced by the buttons - and the other items in the list (skip, exit) stay in the list but the actions will be triggered without having to click on the evaluate button, since we remove it.

Is that clear ?

In term of code, the widget that take care of this is located here:

https://github.com/mozilla/mozregression/blob/73f2c4f70d589e87e4afdcd5203a306ce1d53d6a/gui/mozregui/report_delegate.py#L9

and it has a .ui file associated here:

https://github.com/mozilla/mozregression/blob/73f2c4f70d589e87e4afdcd5203a306ce1d53d6a/gui/mozregui/ui/ask_verdict.ui

As you may know, the .ui file should be edited with the QT designer tool (or QT creator). I allow to draw the ui, and it will be compiled in python code then used in the class with this call:

https://github.com/mozilla/mozregression/blob/73f2c4f70d589e87e4afdcd5203a306ce1d53d6a/gui/mozregui/report_delegate.py#L16-L17

So basically, you should change the .ui and make modifications in the AskVerdict class (report_model.py)

You should take care of not breaking the interface, that is you need to call the got_verdict signal when the user choose a verdict:

https://github.com/mozilla/mozregression/blob/73f2c4f70d589e87e4afdcd5203a306ce1d53d6a/gui/mozregui/report_delegate.py#L51
https://github.com/mozilla/mozregression/blob/73f2c4f70d589e87e4afdcd5203a306ce1d53d6a/gui/mozregui/report_delegate.py#L44

I suggest you first add some print statements in the code, to understand how that works. :)

I hope this helps! We can always discuss that on irc.

Comment 10

[Wasif Hyder]

Sure thing. I'll explore a bit on my own, then hit you up on IRC tomorrow :) 

Cheers

Comment 11

[Wasif Hyder]

Attachment: Feature 1235003 - Adding good / bad buttons as default

Hi,

As discussed, I've implemented the buttons as requested. The only question I'd have for now is whether tests are needed for this feature, and whether the good / bad buttons need the circle icon on them as well.

Comment 12

[Julien Pagès (:parkouss)]

Attachment: gui-evaluate.png

Hey Wasif,

Your patch looks good!

Yeah, i would like to see colors on the buttons. Also, if you can remove the space between the buttons and the combo, and remove the space inside the buttons that would be great (reduce the padding) - this takes quite some place in the row right now.

Also I wonder if it would be better to rearrange the items in |good|bad|combo with other choices| ?

If you can add some tests easily, please go ahead! I always welcome tests :)


I think it's a good change overall, but I would like to hear from others before we go further with this. Will, Mike, what do you think of the change ? (I attached an image, first line is what we have currently, the second is what we are doing now). So the idea is to show good and bad choices as buttons instead of in the combobox, because those are probably the most used choices. then we have a combobox for other choices. The idea is to reduce the number of clicks needed. Note we may take other approaches, like maybe using keyboard shortcuts.

What do you think guys ?

Comment 13

[Julien Pagès (:parkouss)]

Wasif, I don't want to make you do something we are not sure to take yet (sorry about that!). Maybe you can look at the other bugs in the meantime ?

Comment 14

[Julien Pagès (:parkouss)]

Comment on attachment 8712730 [details] [review]
Feature 1235003 - Adding good / bad buttons as default

I remove the review flag for now.

Also Wasif, when you want to be up to date with mozilla/master (integrate in your local fork the changes from there), you should not merge here, but instead rebase. It will be easier for me to merge, and it is better since you won't have any merge commit in your branch (only your commits "rebased" on top of the latest changes in mozilla/master). Here is a basic workflow that you can use:

> git checkout -b bug-XXX # create and checkout a new branch to work on a bug

Then act as usual, git commit to add new stuff.

When you want to take modification from mozilla/master:
> git pull --rebase https://github.com/mozilla/mozregression master

Note that when you do this, your history will be rewritten, so you have to force push on your distant branch:
> git push -f origin bug-XXX

Comment 15

[Wasif Hyder]

(In reply to Julien Pagès (:parkouss) from comment #14)
> Comment on attachment 8712730 [details] [review]
> Feature 1235003 - Adding good / bad buttons as default
> 
> I remove the review flag for now.
> 
> Also Wasif, when you want to be up to date with mozilla/master (integrate in
> your local fork the changes from there), you should not merge here, but
> instead rebase. It will be easier for me to merge, and it is better since
> you won't have any merge commit in your branch (only your commits "rebased"
> on top of the latest changes in mozilla/master). Here is a basic workflow
> that you can use:
> 
> > git checkout -b bug-XXX # create and checkout a new branch to work on a bug
> 
> Then act as usual, git commit to add new stuff.
> 
> When you want to take modification from mozilla/master:
> > git pull --rebase https://github.com/mozilla/mozregression master
> 
> Note that when you do this, your history will be rewritten, so you have to
> force push on your distant branch:
> > git push -f origin bug-XXX

Noted! I'll keep that in mind for the future. I'll add the colors on the buttons, and then move on to the next bugs.

Comment 16

[MikeLing]

Hi Julien and Wasif, sorry for the late reply. I had to catch the train yesterday and haven't check my email after back to home.

>Yeah, i would like to see colors on the buttons. Also, if you can remove the space between the buttons and >the combo, and remove the space inside the buttons that would be great (reduce the padding) - this takes >quite some place in the row right now.
>Also I wonder if it would be better to rearrange the items in |good|bad|combo with other choices| ?

Yeah, I totally agree with Julien's advice. And thank you Wasif would like to work on it :)

Comment 17

[William Lachance (:wlach)]

I like the buttons! Arguably it might be better to use buttons for all the different options, if they would fit.

Comment 18

[Julien Pagès (:parkouss)]

Ok thanks guys!

Hm Wasif, I still don't see the buttons reordered, nor the colors in the buttons - and there is too much space between the elements (see comment 12). Could you fix that please ? Once done, I'll be happy to r+ this and land your patch.

Comment 19

[Wasif Hyder]

(In reply to Julien Pagès (:parkouss) from comment #18)
> Ok thanks guys!
> 
> Hm Wasif, I still don't see the buttons reordered, nor the colors in the
> buttons - and there is too much space between the elements (see comment 12).
> Could you fix that please ? Once done, I'll be happy to r+ this and land
> your patch.

Hi Julien,

I'll get to work on it immediately. Had been on a flight for the past day, hence the delay. I apologise. I'll have it done asap.

Comment 20

[Julien Pagès (:parkouss)]

Hey, no worry at all Wasif! I thought maybe you forgot to push it, that's all - I did not mean to stress you, sorry for that. :)

Comment 21

[Wasif Hyder]

I've tried to resolve this error on my end, but its taken me too long and is giving me a lot of trouble. Can't move forward without it.

The compilation error is:
Executing `'C:\Python27\python.exe' mozregui/main.py`
Traceback (most recent call last):
  File "mozregui/main.py", line 14, in <module>
    from mozregui.mainwindow import MainWindow  # noqa
  File "D:\0. Wafi Dropbox\Code\Python\Mozilla\mozregression\gui\mozregui\mainwindow.py", line 12, in <module>
    from mozregui.bisection import BisectRunner
  File "D:\0. Wafi Dropbox\Code\Python\Mozilla\mozregression\gui\mozregui\bisection.py", line 11, in <module>
    from mozregression.approx_persist import ApproxPersistChooser
ImportError: No module named approx_persist
ERROR: Command '('C:\\Python27\\python.exe', 'mozregui/main.py')' returned non-zero exit status 1

My previous versions work, but the latest branch I pulled from upstream is not compiling. Is there something I'm doing wrong?

Comment 22

[Wasif Hyder]

I've tried to resolve this error on my end, but its taken me too long and is giving me a lot of trouble. Can't move forward without it.

The compilation error is:
Executing `'C:\Python27\python.exe' mozregui/main.py`
Traceback (most recent call last):
  File "mozregui/main.py", line 14, in <module>
    from mozregui.mainwindow import MainWindow  # noqa
  File "D:\0. Wafi Dropbox\Code\Python\Mozilla\mozregression\gui\mozregui\mainwindow.py", line 12, in <module>
    from mozregui.bisection import BisectRunner
  File "D:\0. Wafi Dropbox\Code\Python\Mozilla\mozregression\gui\mozregui\bisection.py", line 11, in <module>
    from mozregression.approx_persist import ApproxPersistChooser
ImportError: No module named approx_persist
ERROR: Command '('C:\\Python27\\python.exe', 'mozregui/main.py')' returned non-zero exit status 1

My previous versions work, but the latest branch I pulled from upstream is not compiling. Is there something I'm doing wrong?

Comment 23

[Wasif Hyder]

Ignore that, I found a fix.

Comment 24

[Wasif Hyder]

I've added the icons, however they're coming across as gray on the buttons. Any suggestions?

https://github.com/wasifhyder/mozregression/blob/master/gui/mozregui/report_delegate.py

Comment 25

[Julien Pagès (:parkouss)]

Hmm, I see that the color are only built for the verdict combo,

https://github.com/wasifhyder/mozregression/blob/master/gui/mozregui/report_delegate.py#L20

What is in verdict combo is not sufficient to build every colors anymore, because good and bad are out of that combo list. Unless I am missing something ?

Comment 26

[Wasif Hyder]

Added the wrong link. My bad.

https://github.com/wasifhyder/mozregression/blob/bug-1235003/gui/mozregui/report_delegate.py

Here.

Comment 27

[Julien Pagès (:parkouss)]

https://github.com/wasifhyder/mozregression/blob/bug-1235003/gui/mozregui/report_delegate.py#L44

Shouldn't it be:

> self.ui.goodVerdict.setIcon(AskVerdict.icons_cache["good"])
instead of
> self.ui.goodVerdict.setIcon(AskVerdict.icons_cache[text])

From what I can see the variable text comes from the verdict combo. And same thing for "bad". :)

Comment 28

[Wasif Hyder]

Ah :) Thanks for that. I don't know why I didn't see that....

Comment 29

[Wasif Hyder]

Attachment: Bug 1235003 - Adding good / bad buttons and triggering actions on drop down.

Comment 30

[Julien Pagès (:parkouss)]

Comment on attachment 8715602 [details] [review]
Bug 1235003 - Adding good / bad buttons and triggering actions on drop down.

Looks good, thanks!

You missed some empty lines, and flake8 was unhappy (that's why the builders were red on the PR). I fixed that. While I was at it, I made a minor modification to show a non selectable "other..." first item in the combobox. I think it is more intuitive.

Comment 31

[Julien Pagès (:parkouss)]

Ok, we should be good here! I think I may release a new mozregression GUI at the end of the week, or maybe next week (I have to release mozregression non gui quite soon because of recent fennecs builds broken anyway).

Comment 32

[Julien Pagès (:parkouss)]

I forgot the link. Landed in:
https://github.com/mozilla/mozregression/commit/69f1be7dec004493c2f6b1bafffc556de8b09d8a

Comment 33

[Wasif Hyder]

Thanks!! :) Sounds great.

What could I work on next?

Comment 34

[Julien Pagès (:parkouss)]

(In reply to wasif_hyder from comment #33)
> Thanks!! :) Sounds great.
> 
> What could I work on next?

I'm glad you ask!

I would say bug 1241358 and bug 1241376. Pick one (or both!) if you like - just add a comment in the bug and I'll assign them to you. :)