ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
61.16k stars 10.31k forks source link

Proposal: annotate obsolete APIs with deprecation attributes #2699

Open mosra opened 5 years ago

mosra commented 5 years ago

Version/Branch of Dear ImGui:

Version: any Branch: any

Back-end/Renderer/Compiler/OS

Back-ends: any Compiler: any Operating System: any

My Issue/Question:

(Apologies if this was brought up already before, I didn't find any related issues.)

When upgrading to 1.72 I realized my code was accidentally using APIs obsoleted back in 2017 (in particular, ImGuiSetCond_FirstUseEver) and I was happily living in a false sense of security until 1.72 removed the obsolete alias. While I know that there's IMGUI_DISABLE_OBSOLETE_FUNCTIONS, defining that is usually too harsh -- you don't want your code to break every time an API gets obsoleted.

In Magnum I'm annotating deprecated APIs with compiler-specific deprecation attributes. This proved very useful for users over the years and made upgrades much smoother -- instead of code breaking when deprecated API finally gets removed, users get a compiler warning early enough (of course there's an ability to disable all deprecated APIs as well, like ImGui has). The macros work for files, macros, functions, typedefs, C++11 using aliases, enum values and namespaces. The actual self-contained source is here (and human-readable docs with examples here) -- feel free to grab any of it, it's MIT-licensed, but I have no problem with putting those to public domain. All of those are currently tested on C++ compilers back to GCC 4.8 / MSVC 2015 (and I have GCC 4.4 / MSVC 2013 compat in some old branch, if needed), while some (namespace / enum attributes, which need C++11 [[attribute]] syntax) are enabled only on newer versions. Generally, those are all only "nice to have" and get defined to nothing on unknown/unsupported compilers -- so introducing them shouldn't lead to compatibility issues.

I'm opening an issue instead of a PR so I can know your opinion first. Thank you! :)

Standalone, minimal, complete and verifiable example:

A bunch of examples how could this look -- for enums:

#ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS
    , ImGuiStyleVar_Count_ IMGUI_DEPRECATED_ENUM("use ImGuiStyleVar_COUNT instead") 
        = ImGuiStyleVar_COUNT // [renamed in 1.60]
    , ImGuiStyleVar_ChildWindowRounding IMGUI_DEPRECATED_ENUM("use ImGuiStyleVar_ChildRounding instead") 
        = ImGuiStyleVar_ChildRounding // [renamed in 1.53]
#endif

For functions:

#ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS
namespace ImGui
{
    // OBSOLETED in 1.72 (from July 2019)
    static inline void IMGUI_DEPRECATED("use SetCursorPosX(GetCursorPosX() + GetTreeNodeToLabelSpacing() instead") TreeAdvanceToLabelPos() {
        SetCursorPosX(GetCursorPosX() + GetTreeNodeToLabelSpacing());
    }
    ....

... typedefs:

#ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS
    typedef IMGUI_DEPRECATED("use ImFontAtlasCustomRect instead") ImFontAtlasCustomRect CustomRect;
        // OBSOLETED in 1.72+
ocornut commented 5 years ago

Thank you Vladimir, I appreciate the detailed post.

Some thoughs (not a firm answer at all, just wanted to lay down my thoughts first)

One issue I have with this kind of behavior is that it would turn dear imgui into the kind of library that starts throwing warnings at your face when you update. I understand that's the precise intent here... but I am worried for less agile users this would create constant small friction and confusion. The last thing I want is people associating dear imgui with a deluge of warnings and have them avoid updating.

The current cut-off system provides users with more leeway to fix their stuff and reduce friction with code sharing (public or accross codebases). By the time they reach the cut-off, there are defacto more internet references to pull from and they are a little more likely to have fixed it already.

I was happily living in a false sense of security until 1.72 removed the obsolete alias.

Probably took you a few minutes to fix? I think experienced users would probably want to enable IMGUI_DISABLE_OBSOLETE_FUNCTIONS everytime they update.

Your specific case is different because you are providing a middleware for others to use. so if anything you wouldn't want to ship with IMGUI_DISABLE_OBSOLETE_FUNCTIONS so there's added gymnastic :(

As a side note, I don't necessarily intent to remove everything after ~2 years, but I'll prioritize cases where the aliasing add confusion in the API surface. In particular, the four IsXXX functions obsoleted in 1.51 added a bunch of very misleading attractive API surface.

mosra commented 5 years ago

it would turn dear imgui into the kind of library that starts throwing warnings at your face when you update [...] I am worried for less agile users this would create constant small friction and confusion

Not my personal experience (but you have a far broader user base, so that might differ in your case). Mostly users react to those warnings with "oh, thanks for letting me know, I'll adapt the code when I have time" but I have to emphasise that the deprecation warning needs a clear message saying what should they use instead. Otherwise it's really just friction and confusion (for example, ffmpeg emits message-less deprecation warnings and figuring out how to update the code is a pain).

Probably took you a few minutes to fix?

With a deprecation warning saying "use ImGuiCond_FirstUseEver instead" it would take me 10 seconds :) But yes, this is not a critical issue by any means and I understand your concerns.

Among other ideas I had was introducing a new IMGUI_WARN_ON_OBSOLETE_FUNCTIONS macro (or, rather, its inverse, IMGUI_DONT_WARN_ON_OBSOLETE_FUNCTIONS, because you most definitely wouldn't want to spend time writing deprecation messages the users won't see by default), but that's complicating things a bit too much I think.

In any case this means additonal work on your side -- ensuring that you properly annotate each obsolete API. But maybe you then wouldn't need to have so detailed changelogs as this information would be already in the code itself.