Re: [tor-bugs] #28329 [Applications/Tor Browser]: Design TBA+Orbot configuration UI/UX

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#28329: Design TBA+Orbot configuration UI/UX
-------------------------------------------------+-------------------------
 Reporter:  sysrqb                               |          Owner:  tbb-
                                                 |  team
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Very High                            |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, ux-team, TBA-a3,         |  Actual Points:
  tbb-8.5-must-alpha, TorBrowserTeam201904       |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor8
-------------------------------------------------+-------------------------

Comment (by sysrqb):

 Replying to [comment:88 gk]:
 > 364e234d66ec101995e1fae1254e632fd23eb69b -- not okay
 >
 > `currentTop` does not really seem to be needed.

 Deleted now.

 >
 > {{{
 >         final int expectedHeight = (int)
 (currentWidth*imgHeight/imgWidth);
 >         final int expectedWidth = (int)
 (currentHeight*imgWidth/imgHeight);
 > }}}
 > You already calculated the ratios and saved them in variables, no need
 to
 > calculate them again here (and elsewhere!).
 >

 I deleted the ratio variables because I found the syntax confusing due to
 additional type casting and needing more nested parentheses. This was
 included in the accidental-fixup commit, but I can revert this change if
 you think the ratio variables are more readable.

 > `// Add a classback for` I guess you meant "callback"?

 I sure do.

 >
 > {{{
 > +        //     Adjust the width when the current width is greater than
 the expected
 > +        //     width.
 > +        //   - The opposite is likely true when the device in in
 landscape mode with
 > +        //     respect to the height and width. Adjust the height when
 it is greater
 > +        //     than the expected height.
 > +        //
 > +        // Subtract 1 from the expected value as a way of accounting
 for rounding
 > +        // error.
 > +        if (currentWidth < (expectedWidth - 1)) {
 > +            newHeight = expectedHeight;
 > +        } else if (currentHeight < (expectedHeight - 1)) {
 > +            newWidth = expectedWidth;
 > +        }
 > }}}
 > That part is a bit dense. I was wondering for a while where the width
 gets actually adjusted when it is greater than expected as neither of your
 two if-clauses is actually checking that as your comment suggests. So,
 ideally your comment would at least explain why this scenario falls into
 the `currentHeight < (expectedHeight -1)` check.


 I'll add more details in the comment.

 >
 > s/in in/is in/

 ack.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28329#comment:91>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs