| [ Return to Bugs & Features | SVN ⇄ GIT ]
STR #3277
Application: | FLTK Library |
Status: | 1 - Closed w/Resolution |
Priority: | 1 - Request for Enhancement, e.g. asking for a feature |
Scope: | 3 - Applies to all machines and operating systems |
Subsystem: | Core Library |
Summary: | Fl_Text_Display virtual method |
Version: | 1.4-feature |
Created By: | superbem |
Assigned To: | matt |
Fix Version: | 1.4.0 |
Fix Commit: | cdb51045dda7076be33147cc8e9138035ad9b97e |
Update Notification: | |
Trouble Report Files:
Trouble Report Comments:
|
#1 | superbem 16:39 Jan 20, 2016 |
| Could you please turn virtual the function (virtual please)void draw_string(int style, int x, int y, int toX, const char *string, int nChars) const; of the Fl_Text_Display?
Thanks | |
|
#2 | superbem 17:12 Jan 20, 2016 |
| Or just add the feature for background color from Style_Table_Entry as in the attatchement.
Thanks | |
|
#3 | AlbrechtS 10:00 Jan 22, 2016 |
| Thank you very much for the proposal.
I checked 'Fl_Text_Display_draw_string.txt' and found it very interesting. I believe that we can add a patch like yours so that we can have a background color feature in Fl_Text_Display (and in Fl_Text_Editor).
I tested it with our text editor (test/editor.cxx) by setting some background colors, and the results were impressing.
Before I write more about the details, here are some technical comments:
We prefer patches in form of uniform diff files with context. You can create such a patch with "diff -u". Such patches can be easier understood by devs and tend to be more "stable" (i.e. applicable if the underlying source code changes over time). If you can, please send patches with diff -u in the future. Thanks.
It took a while, but I converted your text file to a patch, and I'll upload it as "Fl_Text_Display_draw_string.diff".
I also changed some variable names and reformatted some statements to make it more compliant with the FLTK coding standard. Particularly tab setting must be 8 columns to see and get correct indenting. If you intend to send more patches, please take a look at the coding standard: http://www.fltk.org/cmp.php#CODING_STANDARDS
All contributions are very much appreciated. TIA.
One question: can we close STR #3159? That one seems to be superseded by this new one. See http://www.fltk.org/str.php?L3159 | |
|
#4 | AlbrechtS 10:18 Jan 22, 2016 |
| More patch details:
Unfortunately we can't just make a method virtual in a patch release (1.3.x) because this would break the ABI. Generally we can do this though in the next minor release (1.4.0) - or we can use ABI guards in the code, so that you would need to configure the ABI version for this new feature.
I believe the better solution here would be to apply a patch like yours. It seems to work like intended and is basically simple. The problem with the patch is however that it uses an existing variable in the Style_Table_Entry structure. Although this variable is currently unused (AFAICT) someone else might have used it differently - although that would be at one's own risk.
I think that we should extend the Style_Table_Entry structure and maybe also (!) use the attr structure member to mark which entries in the structure are used (similar to the xlib protocol in many cases). This serves the purpose to distinguish a field (like background color) that is 0 from an unset field. I don't know if this is necessary here, but maybe. Or maybe not, since 0 is a special color that is not relevant here, so we might just do it as in your code: test if the color is non-zero to see if it has a meaning.
That said, extending the Style_Table_Entry structure would also break the ABI, but as I wrote before, this could be done with ABI guard macros.
I'm still thinking about the issue, will maybe test more and prepare a new patch, but please don't hold your breath. I don't know when I'll find more time.
Anyway, comments and proposals welcome... | |
|
#5 | superbem 09:47 Jan 24, 2016 |
| Ok, I'll send the next ones with diff patch.
A thing I don't understand about breaking the abi, I thought It has something to do with software linked to the shared libs of fltk.
Anyway, if it is that, on my understanding, it would be a problem if we are turning a virtual to internal, never the opposite, since a method is not virtual, just internal, the shared lib runs well anyway.
I'm I missing something?
Thanks | |
|
#6 | AlbrechtS 08:11 Jan 25, 2016 |
| The ABI (Application *Binary* Interface) depends on the internal implementation and representation of objects and function calling mechanisms. One of these is an internal table of virtual methods, usually called 'vtab' or similar. Making a method virtual adds on entry to this vtab, and this changes obviously the layout of that table for one class and all classes derived from that class. Hence this breaks the ABI: Programs compile with a newer version can't run with an older shared library and vice versa.
OTOH you can always add non-virtual methods and static class methods, but you must not remove public methods.
http://ispras.linuxbase.org/index.php/ABI_compliance_checker#FAQ https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ | |
|
#7 | superbem 14:51 Apr 16, 2016 |
| I have notest that it wasnt implementented neither in svn, so can you add this please at Fl_Text_Display.H:
+#ifdef DEV_TEST +virtual +#endif void draw_string(int style, int x, int y, int toX, const char *string, int nChars) const;
thanks | |
|
#8 | matt 07:08 Feb 05, 2019 |
| Anything that keeps us from putting this into 1.4? | |
|
#9 | AlbrechtS 08:47 Feb 05, 2019 |
| Short question: what?
Longer question: Did you mean the proposed patch (https://www.fltk.org/strfiles/3277/Fl_Text_Display_draw_string.diff) or just to make draw_string() virtual?
There's IMHO nothing that prevents us from making draw_string() virtual, but the patch has not been verified. I just converted the given text file (#1) to a patch (#2), see comment #3 and my comment #4:
> The problem with the patch is however that it uses an existing > variable in the Style_Table_Entry structure. Although this > variable is currently unused (AFAICT) someone else might have > used it differently - although that would be at one's own risk. > > I think that we should extend the Style_Table_Entry structure and ...
@Matt: That said, you are the one who knows Fl_Text_Display best, so I'd be glad if you could take over this STR and decide yourself what to do.
I deassigned myself, please feel free to take over... TIA. | |
|
#10 | AlbrechtS 09:18 Feb 05, 2019 |
| Bumped to 1.4-feature.
In 1.4 we wouldn't need the #ifdef DEV_TEST anyway, we could just make it virtual (if nothing else would break with this, of course). | |
|
#11 | matt 14:58 Jan 27, 2022 |
| Fixed in Git repository. | |
[ Return to Bugs & Features ]
|
| |