Summary:

Added changes in reviews.js to support the new dirtyStateChanged signal in djblets

Review Request #3033 — Created March 31, 2012 and discarded Aug. 5, 2013, 1:29 p.m.

Information
Owner:	jimrrchen
Repository:	Review Board
Branch:
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

Added changes in reviews.js to support the new dirtyStateChanged signal in djblets

Testing Done

Tested in Chrome 17.0.963.79 and Firefox 11.0

Issues

Description	From	Last Updated
Since this function appears multiple times, maybe its better to take it out as a separate function. Also, it might …	ME medanat	March 31, 2012, 8:51 a.m.
This should be above all usage of the function. Also, "isDirty".	chipx86	March 31, 2012, 6:34 p.m.
I'm not a fan of this type of usage. Please use if statements instead.	chipx86	March 31, 2012, 6:34 p.m.

I see this function being bind to many elements, are you sure you need all of them? Are there any overlaps in your even listeners?

Yazan Medanat

jimrrchen March 31, 2012, 8:49 a.m.

yes..I think we need all of them. The new dirtyStateChanged signal is supposed to replace BeginEdit, cancel and complete signals.
dirtyStateChanged would overlap a bit with cancel and complete, but Christian is fine about it...I think

medanat March 31, 2012, 8:52 a.m.

My concern is an overlap will trigger gEditCount++ twice, could this be an issue?

jimrrchen March 31, 2012, 8:56 a.m.

It wouldn't be because I removed gEditCount++/gEditCount-- in the handler of BeginEdit/cancel/complete, in fact, I removed the handler of BeginEdit completely

reviewboard/static/rb/js/reviews.js (Diff revision 1)

Show all issues

Since this function appears multiple times, maybe its better to take it out as a separate function.

Also, it might be cleaner to have this as:

is_dirty ? gEditCount++ : gEditCount--;

jimrrchen March 31, 2012, 8:55 a.m.

Hmm...good call...I agree with changing it to a one-liner...and make a function for it

Change Summary:

refactor code to make a new function handleDirtyStateChanged and make it a one-liner

Diff:

Revision 2 (+12 -52)

Show changes

reviewboard/static/rb/js/reviews.js

reviewboard/static/rb/js/reviews.js (Diff revision 2)

Show all issues

This should be above all usage of the function.

Also, "isDirty".

reviewboard/static/rb/js/reviews.js (Diff revision 2)

Show all issues

I'm not a fan of this type of usage. Please use if statements instead.

Change Summary:

minor update per review comments

Diff:

Revision 3 (+20 -52)

Show changes

reviewboard/static/rb/js/reviews.js

Review Board 6.0 beta 3 (dev)

Added changes in reviews.js to support the new dirtyStateChanged signal in djblets

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded