Do we have tests that attaching backbuffers is illegal in ddraw2/4/7? It would be nice to add them if not.
Yes, corresponding test_surface_attachment() in the later ddraw versions has attempts to attach surfaces the same way which are expected to fail.
I apologize for not being able to think or read very well, but why do we need both is_root and is_chain_start? Why can't we just modify is_root?
'is_root' is what distinguishes app-created surface (complex surface root in a general case) with auto-created attachments. is_chain_start designates the first surface in the constructed chain (which is always is_root but with these additions there can be multiple is_root surfaces). I didn't find a reasonable way to get rid of that which would look like a simplification to me. The flip chain start check is needed in ddraw_surface_cleanup (to know where to stop, we do not want to circle there to the beginning of the chain; if there is another 'is_root' surface it will be deconstructed when released by app). Then, DeleteAttachedSurface is not allowing to delete the linked chain list (returning DDERR_CANNOTDETACHSURFACE; the tests suggest that DDERR_SURFACENOTATTACHED takes precedence and this error code suggests that native ddraw also recognizes chain start as attached from the last element in chain but explicitly refuses to detach it). So it seems to me we really need to know the star t of the chain. That can of course be done differently like storing (yet another) 'first_attached' pointer equivalent but that doesn't seem nicer to me? In the next version I renamed it to is_flip_chain_start.
detach_clear_flags I don't like for reasons that are hard to describe. I think we can instead store the original flags (and I'd say "original_caps") and selectively restore those on detach? Does that work or am I missing something?
Yes, I have that in the next version which I am about to send soon (except for CAPS_FRONTBUFFER flag is not preserved but I am just excluding it from restore flags).
I don't find this sort of thing very idiomatic; I would prefer to just do "if ((caps & 3DDEVICE) || (version == 1 && (caps & PRIMARYSURFACE)))". At that point it probably doesn't need a helper function either.
There is also ddraw-\>flags & DDRAW_NO3D check? It seems like a rather convoluted condition, do you really want to duplicate it? We might avoid fixing up 'caps' variable ofc in favour of explicit use of `force_3ddevice() in wined3d_resource_desc_from_ddraw().` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10992#note_141611