Closed GoogleCodeExporter closed 9 years ago
This should be done transitively. That is, if a || b and b || c then a || c.
Therefore, when the mouse is over "a" then both "b" and "c" should be
highlighted in blue. The same transitive property holds for \not{||}.
In other words, you're highlighting the two equivalence classes of events that
are concurrent/not concurrent with the event being pointed to.
Original comment by bestchai
on 16 Nov 2011 at 12:35
Issue 133 has been merged into this issue.
Original comment by bestchai
on 16 Nov 2011 at 12:44
If I do this, I think that the InvariantsGraph will be presenting too much
information. Maybe we should make separate graphs or tabs for the TO and PO
invariants.
Original comment by T.101.JV
on 20 Nov 2011 at 3:30
I think that it is worth it to try this idea out. We might reconsider this in
the future, and building a PO-specific visualization will require much more
effort than what's proposed here.
For the example that we currently have I don't think that it will be too
overwhelming. In addition, the code already does some invariant filtering
(e.g., chooses the AFby invariant over NCwith when both are true) to diminish
the number of invariants shown to the user.
Original comment by bestchai
on 20 Nov 2011 at 3:56
I left code review comments on revision f90faf7a9f43
Here are some other high-level comments:
- It would help to highlight the background of the InvariantGridLabel
corresponding to NCwith and ACwith invariants when they receive a mouse-over
event. Just like with the other invariant types.
- They should also be highlighted when the mouse-over event is on the
InvariantsGraph -- to link the table with the graph.
- For the TicketReservation example, the buys_c1 || buy_c0 invariant/partition
is not highlighted when mousing over the corresponding invariant in the table.
This works for the search ACwith invariant.
- I think we should use a different set of colors for ACwith and NCwith
invariants -- not red/blue but something different. Otherwise, the color coding
is too ambiguous.
Original comment by bestchai
on 27 Nov 2011 at 11:29
I went and did revision 83db6f20a661 and did not realize you had reviewed
revision f90faf7a9f43. I will expect incremental reviews in the future.
Original comment by T.101.JV
on 28 Nov 2011 at 4:10
Sorry, I thought that this was the revision that completely solved this issue.
In the future I'll refrain from reviewing code unless explicitly prompted by a
request on the issue page.
Original comment by bestchai
on 28 Nov 2011 at 4:37
I left a code review for revision 83db6f20a661. There aren't many comment there
since the bulk of the comments are in revision f90faf7a9f43. You should address
the ones for f90faf7a9f43 first.
I raised this in the comments for revision 83db6f20a661 -- your code seems to
animate ACwith/NCwith invariants across all three columns. The original intent
was to do this for just the middle column. What's your reasoning behind
extending this to all three columns?
Original comment by bestchai
on 28 Nov 2011 at 8:11
[deleted comment]
Addressed reviews in revision 9b99f5e883b1. I did everything except for doing
class renames which I will do in a separate commit for simplicity.
Please review.
Original comment by T.101.JV
on 1 Dec 2011 at 8:48
I fixed some comments in revision cca6628bdf60, outstanding issues are:
1. Class renames from comments in revision f90faf7a9f43
2. Two comments in revision 9b99f5e883b1
Original comment by bestchai
on 3 Dec 2011 at 7:46
Changes in revision 88ff7e54512e. I fixed class renames and addressed your
comments. Please review.
Original comment by T.101.JV
on 4 Dec 2011 at 10:26
Two remaining todos:
1. Add hashCode() to POInvariant (used to be GraphicConcurrentInvariant). This
was mentioned in revision 9b99f5e883b1 review.
2. Add a comment to data structures under "/* Event columns */" and "/* Event
columns *" in InvariantsGraph to indicate that they are not used by any of the
code. Yes, the unused warning is gone but that doesn't change the fact that
they are not actually used. It helps to document this for later.
Original comment by bestchai
on 5 Dec 2011 at 2:29
Addressed remaining todos in revision 1d25d77638d7.
Original comment by T.101.JV
on 5 Dec 2011 at 3:20
Looks like the rename refactoring caused some formatting changes. I can
reformat everything in another commit.
Original comment by T.101.JV
on 5 Dec 2011 at 3:26
Merged into default in revision ec83606d0235.
Original comment by bestchai
on 5 Dec 2011 at 3:56
Original issue reported on code.google.com by
T.101.JV
on 15 Nov 2011 at 9:10