cappuccino / cappuccino

Web Application Framework in JavaScript and Objective-J

https://cappuccino.dev/

GNU Lesser General Public License v2.1

2.2k stars 333 forks source link

CPTableView: Don't update selection etc unless becoming first responder succeeds. [+1] #1530

Closed stewa closed 11 years ago

stewa commented 12 years ago

Fix for issue #1435

cappbot commented 12 years ago

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

daboe01 commented 12 years ago

cappbot commented 12 years ago

Milestone: Someday. Vote: 1. Label: #new. What's next? A reviewer should examine this issue.

ghost commented 12 years ago

+bug +AppKit

needs-review

cappbot commented 12 years ago

Milestone: Someday. Vote: 1. Labels: #needs-review, #new, AppKit, bug. What's next? This issue is pending an architectural or implementation design decision and should be discussed or voted on.

aparajita commented 12 years ago

We can't really accept this without a test app that shows before/after the change.

needs-reduction

cappbot commented 12 years ago

Milestone: Someday. Vote: 1. Labels: #needs-reduction, #needs-review, AppKit, bug. What's next?

This issue is pending an architectural or implementation design decision and should be discussed or voted on.
A minimal test app should be created which demonstrates the concern of this issue in isolation.

stewa commented 12 years ago

Tests/Manual/ArrayController1: 1) select a row start editing a text field 2) select another row => changes are lost without the fix and commited with the fix applied.

primalmotion commented 11 years ago

I cannot reproduce. If I follow your steps, the field is updated without any problem

daboe01 commented 11 years ago

i can reproduce it:

click into Selected Price textfield.
edit the 7 to 8 but keep focus in the textfield (ie. do not press return)
click on the second row. The Price of the first row is still 7. Not so in cocoa.

primalmotion commented 11 years ago

Oh we were talking about the textfield, not the row itself. Ok I can reproduce then.

primalmotion commented 11 years ago

accepted

cappbot commented 11 years ago

Milestone: Someday. Vote: 1. Labels: #accepted, #needs-reduction, AppKit, bug. What's next? A minimal test app should be created which demonstrates the concern of this issue in isolation.

primalmotion commented 11 years ago

Merged. Thanks!

aljungberg commented 11 years ago

fixed

cappbot commented 11 years ago

Milestone: Someday. Vote: 1. Labels: #fixed, AppKit, bug. What's next? This issue is considered successfully resolved.

primalmotion commented 11 years ago

We finally have some concern about this one. It makes the CPTableViewTest.j failing.

Let me copy paste conversation about it here :

primalmotion: this one is the problem
https://github.com/cappuccino/cappuccino/commit/19804720af58d9a4617729b626563a21b8e0eaf5
it makes sense why the test is failing
I think this patch is correct, we just need to update the test

14:07 aljungberg
Hmm.
Well that depends.
Isn't it possible to double click on items when a table is not the first responder?

14:08 primalmotion
it would then become  the first responder at the first click?

14:09 aljungberg
Not if whatever was the first responder didn't resign first responder, right?
Also I wonder what happens if the window is a cppanel which can't become the key window. Do controls become the first responder there?

14:10 primalmotion
hum, and in that case are we able to double click ?

14:10 aljungberg
I don't know

So we don't know :)

accepted

cappbot commented 11 years ago

Milestone: Someday. Vote: 1. Labels: #accepted, AppKit, bug. What's next? A reviewer should examine this issue.

aljungberg commented 11 years ago

Until we have an answer on this I am going to fix the test.

aljungberg commented 11 years ago

I have confirmed that in Cocoa on OS X 10.8 that on a table which refuses to become the first responder it's none the less possible to:

select rows
trigger a single click action
trigger a double click action
drag a row

I tested this by creating a subclass of NSTableView which responds NO to acceptsFirstResponder and which emits a logging message in becomeFirstResponder.

So this change is not entirely incorrect. It's true that the table does try to become the first responder when clicked, but if this fails it still handles mouse actions as normal.

(Note: oddly, triggering editing of a table row seems to make the table view the first responder even if it returns NO for acceptsFirstResponder.)

aljungberg commented 11 years ago

-#fixed

milestone=0.9.6 assignee=aljungberg

cappbot commented 11 years ago

Assignee: aljungberg. Milestone: 0.9.6. Vote: 1. Labels: #accepted, AppKit, bug. What's next? A reviewer should examine this issue.

aljungberg commented 11 years ago

This pull request as described in the title has been reverted. One part of the change still remains though: now the table tries to make itself the first responder earlier than before. Maybe this will resolve the original problem in #1435, although there doesn't seem to be any test for this issue.

+#fixed

cappbot commented 11 years ago

Assignee: aljungberg. Milestone: 0.9.6. Vote: 1. Labels: #fixed, AppKit, bug. What's next? This issue is considered successfully resolved.