flipによってカードのidがつけ変わらない

[yumetodo]

from: #93
reported by @sasanquaneuf

カードの削除はidベースで行われているにも関わらず、 #81 で考慮されていない。
cc: @paihu

mastogetter/js/index.js

Lines 90 to 106 in 5c69869

	function flipCards() {
	const cards = $("cards");
	if (!cards) return;
	let card_nodes = [];
	while (cards.hasChildNodes()) {
	if (cards.firstChild.nodeName !== "#text") {
	card_nodes.push(cards.firstChild);
	}
	cards.removeChild(cards.firstChild);
	}
	if (card_nodes.length === 0) return;
	while (card_nodes.length > 0) {
	cards.appendChild(card_nodes.pop());
	}
	impl.card_list.reverse();
	impl.genPermalink();
	}

[yumetodo]

これ、多分idつけ直すべきだとは思うんですけど、別解として削除のときのdblclickイベントの引数に渡されるDOMから親方向にdata-dblclickable attributeの付いているElementを探査して、それをremoveChildするという手もあります。

後者の場合は #72 と衝突します。

[paihu]

html要素のidがころころかわるのは微妙なので、dblclickされたときに要素が何番目なのか？を確認してリストから消すのがよさそうな気がするんですけど

ちなみに drag & drop で順序変更してもも同じことが起こると思われます。

[sasanquaneuf]

もしidつけ直すとすると、削除したときもidつけ直さないといけないような。
いまcard_listと二重管理しているのが問題という考え方もあるので、パーマリンクの生成をcard_list依存にしないように直して、card_listは使わないというのもあります。
idつけ直しは、今後アクションが増えるたびに注意し続けないといけないので、パッと見ではパーマリンクの生成を見た目ベースにするのが一番楽そうかなあと思いました。
あとは、一覧の生成をcard_listから行うようにちゃんと修正するか。
特にしがらみが無くて私が対応するとしたら前者かなあ（影響箇所ちゃんと見たわけではありません）

[KEINOS]

トゥートの DOM 要素（各々の card）にトゥート ID を属性に埋め込むのって悪手なんですかね？

パーマリンク作成（アップデート）の関数を作った時にも思ったんですが、updatePermalinkFromCardList () 実行時、card_list からではなく、現在の DOM 要素からトゥート ID 引っ張って来れれば、表（おもて）に見えている状態を持ってくるので DOM の順番をいじっても、削除しても、フリップしても、What You See Is What You Get になって少し楽になるのかしら、と思いました。

mastogetter/js/common.js

Lines 74 to 81 in 5c69869

	function updatePermalinkFromCardList() {
	console.log("Updaing permalink from card_list.");
	let permalink = "https://qithub-bot.github.io/mastogetter/p.html?i=" + $("instance").value + "&t=";
	Object.keys(card_list).forEach(function(key) {
	permalink += card_list[key] + ",";
	});
	$("permalink").value = permalink;
	}

[yumetodo]

@KEINOS #90 より、toot idはuniqueにならないのでだめです。

[yumetodo]

いまcard_listと二重管理しているのが問題という考え方もあるので

DOMにあるならすべてそれを読めってことでしょうか？それはそれでdataとviewがぐちゃぐちゃになりそう

[KEINOS]

toot idはuniqueにならないのでだめです。

DOM の識別子としての ID ならユーニークである必要はわかるのですが、トゥートの各カードを作成する際に、<cardElement>.addAttribute("toot-id", toot_id) と新しい属性 "toot-id" を各カードに埋め込むのはユニークである必要はないかと。

なんかあちこちで状態を維持する変数を持っている（二重管理している）ので、現在表示されているカードの DOM を中心にすればわかりやすくなる気がしたのです。

それだと抜本的な仕様変更で、変更箇所が多すぎるということですかね。

[yume-yu]

自分も #94 (comment) の「カードがn番目」の情報を使うのに賛成です。重複や並べ替えがあるのにidを使うのはいろいろ大変そうだと思います。

[yumetodo]

そこまでの抜本的変更するならむしろDOMなんかに状態を一切持たせないで状態変更したらDOMを作り直すぐらいの勢いのほうがよさそう(まあそうすると仮想DOM欲しくなるわけですが)。

まあそういうのは別にIssue立てて話したほうが良さそう。

とりあえずid付け直し案は反対多数で棄却し、eventの引数から持ってくることにしましょうか。

[yumetodo]

で、 これによって #72 との衝突が確定的になったと、あやや。

[sasanquaneuf]

影響箇所の洗い出しを事前にきちんとやってから、修正/判断した方が良いかもしれないですね。
現状、DOMのidを使っているところ、特にDOMのidとカード位置のindexの紐付けを利用しているところが他にないか確認して、見通しを立ててから対応すると、衝突や手戻りが少なくなるかもと思いました。

[paihu]

抜本的な変更をするのも悪くないんですが、とりあえずのバグフィックスとしては

https://github.com/paihu/mastogetter/blob/fix-deletecards/js/common.js#L32-L45

こういうお手軽なのを想像してたんですが

[yumetodo]

え、そんな面倒なことしないでも、"ondbclick"eventの第一引数を親方向にたどれば十分です、もっとシンプルに、高速に書けます。当座は。

[yumetodo]

まあ #72 との衝突は私がなんとかするとしてとりあえずの修正投げますか。作業します。

抜本的対策は引き続き話しましょう。

[yumetodo]

よく見たら衝突するのは #85 でした。まあ衝突も解消しておいたので大丈夫です。

[paihu]

この問題
cards内の要素がid(index)順になっていないので、id(inex)を信じてcard_listから削除すると、permalinkがおかしくなるというものだと認識しているのですが、認識間違ってますかね？

その解決方法として私が示したのは、card_listが壊れない方法です。
#98 の方法ではcard_listは依然としてこわれているのでpermalink生成には使えなくなっています

https://youtu.be/5qU2WDsX_Fc

[yumetodo]

だめでしたね。 @paihu さんの修正が必要なのか。見落としていました。 #98 は閉じました。

[sasanquaneuf]

このissueはまだ残しますか？
個人的には、このissue自体は一旦クローズして、もし抜本的改革をするなら別issueでよいような気がしました。

[yumetodo]

とりあえず閉じます。