Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fix #7161 by only refreshing when needed.#7162

Merged
dangoor merged 2 commits intomasterfrom
dangoor/7161-specrunner-exception
Mar 12, 2014
Merged

Fix #7161 by only refreshing when needed.#7162
dangoor merged 2 commits intomasterfrom
dangoor/7161-specrunner-exception

Conversation

@dangoor
Copy link
Copy Markdown
Contributor

@dangoor dangoor commented Mar 11, 2014

Fix for #7161

Check to see if the sortDirectoriesFirst has actually changed before refresh.

@TomMalbran want to take a look?

Check to see if the sortDirectoriesFirst has actually changed before refresh.
Comment thread src/project/ProjectManager.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to align this =

@TomMalbran
Copy link
Copy Markdown
Contributor

@dangoor It looks good. I though that the "change" event would fire only if the value of the given preference changed. I think it should work like that, since that is what the name implies. Would it make sense if the event would also send the old and new preference values when you specify a preference?

@dangoor
Copy link
Copy Markdown
Contributor Author

dangoor commented Mar 12, 2014

@TomMalbran At one point, the preferences system did work that way, but now with the ability to request prefs with a variety of contexts it's up to the subscriber to decide what to do when there's a possible change and how to best keep track of before/after values. The preference system can easily detect that there might have been a change for a given pref, but knowing for certain whether or not the value changed is not as clear. We could do it for the get("somepref") case, but it gets trickier if the listener cares about some other context.

I had actually put an "important note" in the preferences system docs for this, though I know that's less ideal than an API that's as easy as possible.

@TomMalbran
Copy link
Copy Markdown
Contributor

Ok. Keeping track of the preference isn't that hard anyway, and it only needs to be done for some. I was checking the code and sending the old and new pref, seems to be hard.

Anyway, the fix looks good.

@dangoor
Copy link
Copy Markdown
Contributor Author

dangoor commented Mar 12, 2014

OK, thanks for reviewing. I'll merge based on your review of the change.

dangoor added a commit that referenced this pull request Mar 12, 2014
@dangoor dangoor merged commit c1c05b8 into master Mar 12, 2014
@dangoor dangoor deleted the dangoor/7161-specrunner-exception branch March 12, 2014 02:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants