FLTK logo

STR #3277

FLTK matrix user chat room
(using Element browser app)   FLTK gitter user chat room   GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 Home  |  Articles & FAQs  |  Bugs & Features  |  Documentation  |  Download  |  Screenshots  ]
 

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:

Receive EMails Don't Receive EMails

Trouble Report Files:


Name/Time/Date Filename/Size  
 
#1 superbem
17:10 Jan 20, 2016
Fl_Text_Display_draw_string.txt
4k
 
 
#2 AlbrechtS
10:00 Jan 22, 2016
Fl_Text_Display_draw_string.diff
3k
 
     

Trouble Report Comments:


Name/Time/Date Text  
 
#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 ]

 
 

Comments are owned by the poster. All other content is copyright 1998-2024 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.