#198975: saveValue => historyNavigator.add

[VDisawal]

Fixes #198975.

Problem
Consider how scmService.setValue works

1. It doesnt make any changes when the passed value is same as the value already set, and just returns
2. In case the method is invoked with transient = false, it first replaces the last value of the history navigator with its current _value, it then adds the passed value to the history naviagator
3. At the end, it just updates _value member with the passed-value
4. Note: historyNavigator balances itself by always removing duplicates

Following is a typical user flow and what happens under the hood
Please consider the following two data structures which are used-
HistoryNavigator = [Msg1, Msg2, Msg3, Msg4, Msg5, ]
scmService._value = "<empty string"

1) User enters the commit message 

- setValue gets called with transient = true
- HistoryNavigator => no changes [Msg1, Msg2, Msg3, Msg4, Msg5, ""]
- scmService._value = "New Msg"
     
2) User clicks on commit button
- setValue gets called with transient = false, passed value is an empty string
- HistoryNavigator (last value replaced with _value="NewMsg" and passed-value="empty string" added at the end)
[Msg1, Msg2, Msg3, Msg4, Msg5, ""] => [Msg1, Msg2, Msg3, Msg4, Msg5, NewMsg, ""]
- scmService._value = ""
Notice how the end of history naviagtor is always an empty string

Considering the case of undo commit, a major difference is that this is the only time when setValue is called with transient = false and the passed-value is an actual message (and not an empty string).
So, with undo commit, the undone-commit-message is passed as a value. This in-turn causes the undone-commit message to sit at the end of the history navigator and also in the _value member.

If anytime after this a setValue is called with transient = false, this would cause the last item in history navigator (which is the undone-commit-message) to be overwritten and hence lost.

Here is an example showing the same.
Lets assume the history looks like this [Msg1, Msg2, Msg3, Msg4, Msg5, ]

1) User goes in the history to Msg3
- setValue gets called with transient = true
- HistoryNavigator => no changes [Msg1, Msg2, Msg3, Msg4, Msg5, ""]
- scmService._value = "Msg3"

2) User clicks on commit button
- setValue gets called with transient = false, passed value is an empty string
- HistoryNavigator (last value replaced with _value="Msg3" and passed-value="empty string" added at the end)
[Msg1, Msg2, Msg3, Msg4, Msg5, ""] => [Msg1, Msg2, Msg4, Msg5, Msg3, ""]
- scmService._value = ""

3) User does Undo Commit
- setValue gets called with transient = false, passed value is the undone-commit-message
- HistoryNavigator (last value replaced with _value="" and passed-value="Msg3" added at the end)
[Msg1, Msg2, Msg4, Msg5, Msg3, ""] => [Msg1, Msg2, Msg4, Msg5, "", Msg3]
- scmService._value = "Msg3"

** Notice how Msg3 is now at the end of the queue and would get replaced the next time setValue is called with transient = false. This would happen the next time a message is committed for eg.

4) User goes to Msg5 in the history 
- setValue gets called with transient = true
- HistoryNavigator => no changes [Msg1, Msg2, Msg4, Msg5, "", Msg3]
- scmService._value = "Msg5"

5) User commits
- setValue gets called with transient = false, passed value is an empty string
- HistoryNavigator (last value replaced with _value="" and passed-value="" added at the end)
[Msg1, Msg2, Msg4, Msg5, "", Msg3] => [Msg1, Msg2, Msg4, Msg5, ""]
- scmService._value = ""

Msg3 is lost

Fix
The fix for this would be to modify the implementation of setValue when transient = false. Instead of replacing the last value in historyNavigator with the passed-in value we just need to add the passed-in value as another node. This would handle all the cases.

Testing
Tested these manually-

1. Adding a new commit message and committing. This works perfectly fine.
2. Moving in the history, committing, undoing last commit, and then commiting again doest remove the undone-commit form the history.

Unit Tests
9175 passing (2m)
60 pending

[DanielFrantes]

all looks fine

[fellotin]

Correcciones # 198975 .

Problema Considere cómo funciona scmService.setValue

1. No realiza ningún cambio cuando el valor pasado es el mismo que el valor ya establecido y simplemente devuelve
2. En caso de que el método se invoque con transitorio = falso, primero reemplaza el último valor del navegador de historial con su valor_actual, luego agrega el valor pasado al navegador de historial.
3. Al final, simplemente actualiza el miembro _value con el valor pasado.
4. Nota: HistoryNavigator se equilibra eliminando siempre duplicados

A continuación se muestra un flujo de usuario típico y lo que sucede bajo el capó . Considere las siguientes dos estructuras de datos que se utilizan: HistoryNavigator = [Msg1, Msg2, Msg3, Msg4, Msg5,] scmService._value = "<cadena vacía"

1) User enters the commit message 

- setValue gets called with transient = true
- HistoryNavigator => no changes [Msg1, Msg2, Msg3, Msg4, Msg5, ""]
- scmService._value = "New Msg"
     
2) User clicks on commit button
- setValue gets called with transient = false, passed value is an empty string
- HistoryNavigator (last value replaced with _value="NewMsg" and passed-value="empty string" added at the end)
[Msg1, Msg2, Msg3, Msg4, Msg5, ""] => [Msg1, Msg2, Msg3, Msg4, Msg5, NewMsg, ""]
- scmService._value = ""
Notice how the end of history naviagtor is always an empty string

Teniendo en cuenta el caso de deshacer la confirmación, una diferencia importante es que esta es la única vez que se llama a setValue con transient = false y el valor pasado es un mensaje real (y no una cadena vacía). Entonces, al deshacer la confirmación, el mensaje de confirmación deshecha se pasa como un valor. Esto, a su vez, hace que el mensaje de confirmación deshecha se encuentre al final del navegador de historial y también en el miembro _value.

Si en algún momento después de esto se llama a setValue con transient = false, esto provocaría que el último elemento del navegador histórico (que es el mensaje de confirmación deshecha) se sobrescriba y, por lo tanto, se pierda.

Aquí hay un ejemplo que muestra lo mismo. Supongamos que el historial se ve así [Msg1, Msg2, Msg3, Msg4, Msg5,]

1) User goes in the history to Msg3
- setValue gets called with transient = true
- HistoryNavigator => no changes [Msg1, Msg2, Msg3, Msg4, Msg5, ""]
- scmService._value = "Msg3"

2) User clicks on commit button
- setValue gets called with transient = false, passed value is an empty string
- HistoryNavigator (last value replaced with _value="Msg3" and passed-value="empty string" added at the end)
[Msg1, Msg2, Msg3, Msg4, Msg5, ""] => [Msg1, Msg2, Msg4, Msg5, Msg3, ""]
- scmService._value = ""

3) User does Undo Commit
- setValue gets called with transient = false, passed value is the undone-commit-message
- HistoryNavigator (last value replaced with _value="" and passed-value="Msg3" added at the end)
[Msg1, Msg2, Msg4, Msg5, Msg3, ""] => [Msg1, Msg2, Msg4, Msg5, "", Msg3]
- scmService._value = "Msg3"

** Notice how Msg3 is now at the end of the queue and would get replaced the next time setValue is called with transient = false. This would happen the next time a message is committed for eg.

4) User goes to Msg5 in the history 
- setValue gets called with transient = true
- HistoryNavigator => no changes [Msg1, Msg2, Msg4, Msg5, "", Msg3]
- scmService._value = "Msg5"

5) User commits
- setValue gets called with transient = false, passed value is an empty string
- HistoryNavigator (last value replaced with _value="" and passed-value="" added at the end)
[Msg1, Msg2, Msg4, Msg5, "", Msg3] => [Msg1, Msg2, Msg4, Msg5, ""]
- scmService._value = ""

Msg3 is lost

Solución La solución para esto sería modificar la implementación de setValue cuando transient = false. En lugar de reemplazar el último valor en HistoryNavigator con el valor pasado, solo necesitamos agregar el valor pasado como otro nodo. Esto manejaría todos los casos.

Probando Probé estos manualmente-

1. Agregar un nuevo mensaje de confirmación y confirmar. Esto funciona perfectamente bien.
2. Moverse en el historial, confirmar, deshacer la última confirmación y luego confirmar nuevamente no elimina la confirmación deshecha del historial.

Pruebas Unitarias 9175 aprobadas (2m) 60 pendientes

[joaomoreno]

The problem with this fix is that you could end up saving in history a message which was never used for committing:

1. Start with a history with 3 values: A, B, C. Input should be empty.
2. Type something: Z
3. Press up twice to go to message B
4. Commit

🐛 Now the history will have A, C, Z, B, but Z should not have been stored.

[joaomoreno]

That being said, I don't really have a better idea right now other than a large refactoring, so I'd gladly take that bug over the original data loss bug.