[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
daniel added a comment. I can't think of any use case for multiple non-text content types for the same slot, but I wouldn't dismiss the possibility either. I think we don't have to support this if it complicates things. If it's easy and natural to support it, I don't see a problem with doing so.TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Tgr, danielCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
daniel added a comment. In T174036#4208770, @Anomie wrote: Although a "MultiSlotDifferenceEngine" wouldn't make sense of parts of DifferenceEngine's interface like setContent(). Nor would it make much sense to be returning one of those from a ContentHandler. Yes, it's hack. setContent() would probably just fail for a MultiSlotDifferenceEngine. And no ContentHandler should return such a thing. A full refactoring would of course be much nicer. The question is - what's the "minimal viable step" into the right direction that unblocks us.TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Tgr, danielCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
Anomie added a comment. Although a "MultiSlotDifferenceEngine" wouldn't make sense of parts of DifferenceEngine's interface like setContent(). Nor would it make much sense to be returning one of those from a ContentHandler.TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Tgr, AnomieCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
daniel added a comment. I'd expect the "DifferenceEngine" returned by ContentHandler to diff two Content objects, rather than all Content objects that are part of a revision. Indeed. That wouldn't work anyway, since a different DifferenceEngine may be needed depending on the model. However, as Tgr noted, DifferenceEngine currently also does a lot of other things, like looking up revisions and generating the navigation head. This would need to be refactored and split up. Or we need a MultiSlotDifferenceEngine that uses multiple DifferenceEngines internally. That's not the nicest design, but would cause the least fallout to calling code.TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Tgr, danielCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
Anomie added a comment. I'd expect the "DifferenceEngine" returned by ContentHandler to diff two Content objects, rather than all Content objects that are part of a revision. Chances are that most of the database lookup logic, header formatting, and so on would need to be extracted to a different class, so the object returned by ContentHandler can handle only the actual diff logic.TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Tgr, AnomieCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
Tgr added a comment. It would involve creating new DifferenceEngine instances for each non-main slot, sure. But given that DifferenceEngine does everything including revision lookup, and pretty much all its method are public (and some set public properties as a side effect) having Article handle multiple DifferenceEngine instances would probably break more things. ...well, I guess it depends on BC expectations. Should DifferenceEngine::getDiffBody return the concatenated diff of all slots, or just the main slot and then another DifferenceEngine method would create a diff for a specified slot, with Article calling that for all other slots? Ie. do we want to include all slots wherever diffs are shown, or just for action="">TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: TgrCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
Anomie added a comment. Trying to have DifferenceEngine iterate over Content objects doesn't seem too workable. Better, IMO, for most cases would be to have a difference engine per slot generate that slot's diff and then combine them elsewhere.TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Tgr, AnomieCc: Anomie, Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T174036: Allow the diffs to show multiple slots [MCR]
Tgr added a comment. DifferenceEngine is a god class which does everything from interpreting query parameters for selecting the target revision to direct DB queries to table formatting to writing to OutputPage. Also extensions can replace it with a subclass. So a proper solution will involve a full rewrite which splits DifferenceEngine into a controller that determines which slots to diff, the actual diff generation code, and a layout generator which does headings and whatnot. Also probably some kind of composition logic that might vary the same way the composition logic for content rendering does. And BC shims since at least the diff generation code (but probably also some of the layout) lives in the extensions. That's probably way too much for phase 1. A hacky solution that just makes a few DifferenceEngine methods iterate through multiple contents does not seem too hard; some questions that came up in the process: The DifferenceEngine is selected by the ContentHandler, even though the old and new content is not guaranteed to be the same content type. (As a quirk of that, oldid=1&diff=next and oldid=2&diff=prev might in theory end up being completely different renderings.) There are multiple ways of handling that with slots: Make diff engine selection the responsibility of the slot handler instead. IMO the sanest approach; could get messy if multiple extensions share a slot but that seems like an unusual thing to do. Force non-main slots to always have the same content model. There are use cases that would be prevented by that (e.g. wikitext vs. Markdown for the documentation slot). Keep the current behavior (maybe make it less chaotic by always using the old revision to select the engine). Currently when diffing the first revision to its predecessor, no diff is shown, just an error message. When looking at a diff adding a new slot, that seems unhelpful. So we might need new DifferenceEngine functionality for generating a "null diff". Currently the DifferenceEngine can personalize the diff header (tools like undo or link to previous). How will that look when there is a separate engine for each changed slot? TASK DETAILhttps://phabricator.wikimedia.org/T174036EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: TgrCc: Liuxinyu970226, TomT0m, Smalyshev, Lokal_Profil, -jem-, Aklapper, daniel, Lahi, PDrouin-WMF, Gq86, E1presidente, Ramsey-WMF, Cparle, SandraF_WMF, GoranSMilovanovic, QZanden, Tramullas, Acer, LawExplorer, JJMC89, Agabi10, Susannaanas, Aschroet, Fjalapeno, Jane023, Wikidata-bugs, PKM, Base, matthiasmullie, aude, Ricordisamoa, Lydia_Pintscher, Fabrice_Florin, Raymond, Anomie, Steinsplitter, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
