Community
Participate
Working Groups
Build Identifier: Version: 3.7.0 Build id: I20110613-1736 This is a feature request for functionality which enhances the work already done to fix Bug 307311. The fix for Bug 307311 brought general support to display non-ASCII strings correctly within the debugger. The problem however is that it can only be guessed which character encoding format is actually used by the program which is being debugged. This best guess is to use the current platform's default charset, but this might not actually be the case. It therefore would make sense to allow the user to tell the debugger which charset is used by his program, so that the debugger can display strings correctly. Note that a user only has to care about this if he wants to debug a program which uses a different charset than the platform's default charset. Such a user preference option would go in line with the already existing general C/C++ debug format settings found at Preferences -> C/C++ -> Debug. A proposed GUI of this user preference option can be seen within the provided screenshot draft. The programmatic implementation is very straightforward, as only a few very simple gdb commands need to be used. They however are only supported by gdb 7.0 or later. Reproducible: Always
Created attachment 210461 [details] Draft for GUI of this user preference option
Created attachment 210464 [details] Draft 2 for GUI of this user preference option
(In reply to comment #1) > Created attachment 210461 [details] > Draft for GUI of this user preference option Nice. Let me explain our current situation with preferences. Since we migrated from CDI to DSF for the GDB integration, we have inconsistent preferences. The DSF debugger does not currently use the Preferences->C/C++/Debug page. If we are to add something to it, we should honor the other preferences of that page. I think this would be a nice enhancement in itself. The problem happens if some of the preferences on that page don't make sense for DSF... What to do then? So, there is a little complexity there, but starting to cleanup our preferences would be a nice step forward. (In reply to comment #2) > Created attachment 210464 [details] > Draft 2 for GUI of this user preference option Nice also. We would need this menu in each view that shows strings to the user: Variables, Expressions, Registers?, Memory
> The DSF debugger does not currently use the Preferences->C/C++/Debug page. Mmmh. So the DSF debugger only uses the little menu shown in GUI draft 2 to allow the user to change the variable / expression / register format, while CDI only uses the preferences page instead? Is that right? > If we are to add something to it, we should > honor the other preferences of that page. I think this would be a nice > enhancement in itself. You mean honor them by DSF, right? That would certainly be a good thing. It's confusing for users to have a preferences page which has no effect (I don't assume many users are aware of the fact that those settings are only valid for CDI). > The problem happens if some of the preferences on that > page don't make sense for DSF... What to do then? Well, I'd actually say that all of the preferences on that debug page make sense for DSF as well. (OK, I'm not sure about the "Show source files in binaries" option.) But the variable / expression / register format is already used in DSF as well. And you said about the currently existing "Character encoding" preference that > it is used in CDI for knowing how wide characters should be decoded (quoted from Bug 307311#c31). So this preference option actually corresponds to the proposed DSF "Wide character encoding" preference. So I think that this preferences page could be used by DSF very well to allow the user to change the number and text format in the same way as the little popup menu is used right now in DSF. So we would have two GUI "views" (the popup menu and the preferences page) on the same preferences "model" here (in the sense of MVC). > We would need this menu in each view that shows strings to the user: Variables, > Expressions, Registers?, Memory Basically yes. But I wouldn't add it to the register and memory views though. The register view doesn't actually display any strings since it doesn't dereference any char or wchar_t pointers (or am I wrong about that?). And the memory view is more a HEX viewer in my opinion, allowing you to see individual bytes instead of decoded strings.
(In reply to comment #4) > > The DSF debugger does not currently use the Preferences->C/C++/Debug page. > > Mmmh. So the DSF debugger only uses the little menu shown in GUI draft 2 to > allow the user to change the variable / expression / register format, while CDI > only uses the preferences page instead? Is that right? I believe so. In fact, the CDI preference only affects a new launch; existing launches are not updated right away. I don't like that very much. > > If we are to add something to it, we should > > honor the other preferences of that page. I think this would be a nice > > enhancement in itself. > > You mean honor them by DSF, right? That would certainly be a good thing. It's > confusing for users to have a preferences page which has no effect (I don't > assume many users are aware of the fact that those settings are only valid for > CDI). You are absolutely right about that. Our handling of preferences has really suffered by having both DSF and CDI. Someone should really sort that out. But I think a big difference can be made by this bug because it is the the Debug preference page that can really be confusing since it does not apply to the default DSF debugger. > > The problem happens if some of the preferences on that > > page don't make sense for DSF... What to do then? > > Well, I'd actually say that all of the preferences on that debug page make > sense for DSF as well. (OK, I'm not sure about the "Show source files in > binaries" option.) We can look into that and figure out how to handle it properly. We can even move it to a CDI-specific preference page if we want. > But the variable / expression / register format is already > used in DSF as well. And you said about the currently existing "Character > encoding" preference that > > > it is used in CDI for knowing how wide characters should be decoded > > (quoted from Bug 307311#c31). So this preference option actually corresponds to > the proposed DSF "Wide character encoding" preference. > > So I think that this preferences page could be used by DSF very well to allow > the user to change the number and text format in the same way as the little > popup menu is used right now in DSF. So we would have two GUI "views" (the > popup menu and the preferences page) on the same preferences "model" here (in > the sense of MVC). I would lean towards having only the pop-up menu. Less preferences is better, they only confuse the user. But for CDI, we probably have to keep the preference page, so we might be stuck with it. > > We would need this menu in each view that shows strings to the user: Variables, > > Expressions, Registers?, Memory > > Basically yes. But I wouldn't add it to the register and memory views though. > The register view doesn't actually display any strings since it doesn't > dereference any char or wchar_t pointers (or am I wrong about that?). And the > memory view is more a HEX viewer in my opinion, allowing you to see individual > bytes instead of decoded strings. Makes sense.
> The Debug preference page can really be confusing since it does not apply > to the default DSF debugger. > But for CDI, we probably have to keep the preference page, so we might be > stuck with it. Well, putting this together actually means that we only have the following options: a) DSF uses the pop-up menu only and CDI uses the preferences page only (status quo). This however is really confusing for users. Not a good solution. b) DSF uses the pop-up menu and the preferences page as well, and keeps the GUI synchronized between those two. CDI uses the preferences page only. Still not an ideal solution since you have two separate GUIs for the same thing, and because changes made in the DSF pop-up menu would affect the settings for CDI as well. This however is still better (i.e. less confusing) than option a) in my opinion. > I would lean towards having only the pop-up menu. Yes, but I thought removing the preferences page would not be an option. > > OK, I'm not sure about the "Show source files in > > binaries" option. > > We can look into that and figure out how to handle it properly. Sure.
(In reply to comment #6) > > The Debug preference page can really be confusing since it does not apply > > to the default DSF debugger. > > > But for CDI, we probably have to keep the preference page, so we might be > > stuck with it. > > Well, putting this together actually means that we only have the following > options: > > a) DSF uses the pop-up menu only and CDI uses the preferences page only (status > quo). This however is really confusing for users. Not a good solution. > > b) DSF uses the pop-up menu and the preferences page as well, and keeps the GUI > synchronized between those two. CDI uses the preferences page only. Still not > an ideal solution since you have two separate GUIs for the same thing, and > because changes made in the DSF pop-up menu would affect the settings for CDI > as well. This however is still better (i.e. less confusing) than option a) in > my opinion. There is also: c) implement the pop-up menu for CDI and remove the preference page altogether. I don't know how much work that would be. Also, I'm not as knowledgeable about CDI, so making changes there may take longer to get approved, as we'll need input from other committers. Since this is user-facing, I suggest you ask people's opinion on cdt-dev. If someone feels strongly enough to help change CDI, we can go with c), if not, we'll have to go with b), which is still much better than status quo. > > I would lean towards having only the pop-up menu. > > Yes, but I thought removing the preferences page would not be an option. If we have the pop-up that may be ok, although we'd need to get confirmation from the mailing list.
(In reply to comment #7) > There is also: > c) implement the pop-up menu for CDI and remove the preference page altogether. Right. That would be the best way to go. > Since this is user-facing, I suggest you ask people's opinion on cdt-dev. If > someone feels strongly enough to help change CDI, we can go with c), if not, > we'll have to go with b), which is still much better than status quo. OK thanks, I'll ask on cdt-dev.
(In reply to comment #7) > There is also: > c) implement the pop-up menu for CDI and remove the preference page altogether. > I don't know how much work that would be. Also, I'm not as knowledgeable > about CDI, so making changes there may take longer to get approved, as we'll > need input from other committers. > As far as I understand the system menu in a view is used to set the preferences for this particular view. You can have a second view with different preferences. The preference page allows user to modify global preferences, so removing it would make the global preferences associated with the page inaccessible.
(In reply to comment #9) > As far as I understand the system menu in a view is used to set the preferences > for this particular view. You can have a second view with different > preferences. The preference page allows user to modify global preferences, so > removing it would make the global preferences associated with the page > inaccessible. You are right. If I clone the variables view, I can have different number format for each view. The global preference would then be used to select the format for a view that will newly be opened? I wonder if that is really essential for the user? I mean, is it worth having an extra preference page? What we could do is, when we open a new view, we use the format of the latest pop-menu selection (which seems to be what we are doing now, anyway). Or maybe there is a better use-case than what I'm imagining?
(In reply to comment #10) > The global preference would then be used to select the format for a view that > will newly be opened? > Yes, that's how it is supposed to work. > I wonder if that is really essential for the user? I mean, is it worth having > an extra preference page? What we could do is, when we open a new view, we use > the format of the latest pop-menu selection (which seems to be what we are > doing now, anyway). > > Or maybe there is a better use-case than what I'm imagining? Setting the format for the Registers view to 'hexadecimal' was a common case for several vendors.
(In reply to comment #11) > (In reply to comment #10) > > The global preference would then be used to select the format for a view that > > will newly be opened? > > > > Yes, that's how it is supposed to work. > > Setting the format for the Registers view to 'hexadecimal' was a common case > for several vendors. Ok, so there needed to be a way to set the format for all values of the view. CDI does not have a global view menu for that, so using a global preference was a valid solution. However, wouldn't a global view menu have achieved the same thing? I'm not saying it is a better solution, but I'm trying to understand if it would have served the same purpose. Especially until Indigo, we didn't support cloning views, so a view menu would have applied to the single registers view, no? P.S. I noticed that CDI allows to set the format for each individual variable (using the context-menu). DSF does not support that...
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > The global preference would then be used to select the format for a view that > > > will newly be opened? > > > > > > > Yes, that's how it is supposed to work. > > > > Setting the format for the Registers view to 'hexadecimal' was a common case > > for several vendors. > > Ok, so there needed to be a way to set the format for all values of the view. > CDI does not have a global view menu for that, so using a global preference was > a valid solution. However, wouldn't a global view menu have achieved the same > thing? I'm not saying it is a better solution, but I'm trying to understand if > it would have served the same purpose. > > Especially until Indigo, we didn't support cloning views, so a view menu would > have applied to the single registers view, no? > I guess so. > > P.S. I noticed that CDI allows to set the format for each individual variable > (using the context-menu). DSF does not support that... In fact it wasn't a very good idea, especially for local variables. Users expect the individual formats to be persistent at least during the session but it is hard to implement.
(In reply to comment #13) > > Especially until Indigo, we didn't support cloning views, so a view menu would > > have applied to the single registers view, no? > > > > I guess so. If we can confirm that a user/vendor can set a default format for a view using the view menu, even if the view is closed, re-opened, etc, it would indicate that the global preference is a little redundant. Mikhail, would you be comfortable if we removed the global preference in that case, or would you prefer to keep it and apply it to DSF anyway? Depending on the amount of work required to add a view menu for CDI, we may choose to do that anyway, but it is good to know what our options are. > > P.S. I noticed that CDI allows to set the format for each individual variable > > (using the context-menu). DSF does not support that... > > In fact it wasn't a very good idea, especially for local variables. Users > expect the individual formats to be persistent at least during the session but > it is hard to implement. Thanks, that's good to know. Less work :)
(In reply to comment #14) > Mikhail, would you be comfortable if we removed the global preference in that > case, or would you prefer to keep it and apply it to DSF anyway? > > Depending on the amount of work required to add a view menu for CDI, we may > choose to do that anyway, but it is good to know what our options are. > Marc, ideally I would like to keep the global preferences and apply it to DSF as well as moving the UI to the "GDB" preference page. But it needs resources that we don't have.
(In reply to comment #15) > Marc, ideally I would like to keep the global preferences and apply it to DSF Ok. Mathias has offered to look at that. > as well as moving the UI to the "GDB" preference page. But it needs resources > that we don't have. I know all the DSF preferences are in that "GDB" page, but do we need to continue along those lines? Why not keep the main Debug preference page as it is and have it work for DSF as it already does for CDI?
(In reply to comment #16) > (In reply to comment #15) > > > Marc, ideally I would like to keep the global preferences and apply it to DSF > > Ok. Mathias has offered to look at that. > Great! > > as well as moving the UI to the "GDB" preference page. But it needs resources > > that we don't have. > > I know all the DSF preferences are in that "GDB" page, but do we need to > continue along those lines? Why not keep the main Debug preference page as it > is and have it work for DSF as it already does for CDI? Sorry, I thought it was on "GDB/MI" page. Of course, we should just keep it.
So, to clearly state what we aim for: - Have both CDI and DSF use the same Debug preference page. If there are items that don't make sense for DSF, we'll have to address that. - The preference page should not be synchronized with the view menu. Instead, it should be used when opening/cloning a new view. The view menu overrides the global preference. Mathias, how does that sound?
(In reply to comment #18) > So, to clearly state what we aim for: > > - Have both CDI and DSF use the same Debug preference page. If there are items > that don't make sense for DSF, we'll have to address that. DSF can just ignore preferences which don't apply to it, as it does now. Or is there any other sensible way for handling this? > - The preference page should not be synchronized with the view menu. Instead, > it should be used when opening/cloning a new view. The view menu overrides the > global preference. > > Mathias, how does that sound? I'd say, this sounds really good! So this means that the Debug preferences page could be changed to look something like shown in GUI draft 1, right? What I like about this solution is that we don't really have to change the CDI implementation. The only thing which would concern CDI as well is that it would be necessary to add a "Default" option to the existing "Character encoding" preference. Mikhail, do you see any problem with that? Furthermore, can you confirm that this preference is used to decode wchar_t* strings only, but not ordinary char* strings?
(In reply to comment #19) > What I like about this solution is that we don't really have to change the CDI > implementation. The only thing which would concern CDI as well is that it would > be necessary to add a "Default" option to the existing "Character encoding" > preference. Mikhail, do you see any problem with that? > I don't see any problem. > Furthermore, can you confirm that this preference is used to decode wchar_t* > strings only, but not ordinary char* strings? It looks like that according to the code. I am not familiar with the wchar support part, it was added by Nokia.
(In reply to comment #20) > > Furthermore, can you confirm that this preference is used to decode wchar_t* > > strings only, but not ordinary char* strings? > > It looks like that according to the code. I am not familiar with the wchar > support part, it was added by Nokia. Ok, so this means that this preference is actually the "wide character encoding" preference. To summarize, I'd then suggest to make the following changes: - Have both CDI and DSF use the same Debug preference page for determining the default values when opening a new view. In DSF, the popup menu overrides the preference page. - Rename the existing "character encoding" preference to "wide character encoding", since it's actually used to decode wchar_t strings. - Add a new "character encoding" preference which is used for decoding ordinary char strings. This is only used by DSF, and ignored by CDI. - Add some sort of default option to both of these encoding preferences. I'd suggest to reuse the same GUI as for example on the General -> Workspace preferences page, similar to what can be seen in GUI draft 1. - Add both the "character encoding" as well as the "wide character encoding" preferences to a submenu of the popup menu of the Variables and Expressions views in DSF, similar to what can be seen in GUI draft 2. Mikhail, Marc, are you OK with all of these changes?
(In reply to comment #19) > DSF can just ignore preferences which don't apply to it, as it does now. Or is > there any other sensible way for handling this? We could move the CDI-only options to a CDI-only preference page, like the GDB MI page. But let's cross that bridge if we get there. > > - The preference page should not be synchronized with the view menu. Instead, > > it should be used when opening/cloning a new view. The view menu overrides the > > global preference. > > > > Mathias, how does that sound? > > > I'd say, this sounds really good! So this means that the Debug preferences page > could be changed to look something like shown in GUI draft 1, right? Looks good to me. Does it fall under the "Opened view default settings" or should we create another section in the preference page? (In reply to comment #21) > To summarize, I'd then suggest to make the following changes: > > - Have both CDI and DSF use the same Debug preference page for determining the > default values when opening a new view. In DSF, the popup menu overrides the > preference page. ok > - Rename the existing "character encoding" preference to "wide character > encoding", since it's actually used to decode wchar_t strings. If it's ok with Mikhail, it's ok with me > - Add a new "character encoding" preference which is used for decoding ordinary > char strings. This is only used by DSF, and ignored by CDI. I think this is ok. We should strive to avoid breaking CDI, but we don't have to enhance it. > - Add some sort of default option to both of these encoding preferences. I'd > suggest to reuse the same GUI as for example on the General -> Workspace > preferences page, similar to what can be seen in GUI draft 1. I wonder why have a default entry? Why not just add the string "(Default)" next to the default choice, and only have one drop-down box to make our selection? Are we just going for consistency with General -> Workspace? (which could be a good enough reason, although it wastes space). > - Add both the "character encoding" as well as the "wide character encoding" > preferences to a submenu of the popup menu of the Variables and Expressions > views in DSF, similar to what can be seen in GUI draft 2. Ok Thanks for tackling this!
(In reply to comment #21) > Mikhail, Marc, are you OK with all of these changes? Sounds good to me. Thanks for volunteering.
(In reply to comment #21) > (In reply to comment #20) > > > Furthermore, can you confirm that this preference is used to decode wchar_t* > > > strings only, but not ordinary char* strings? > > > > It looks like that according to the code. I am not familiar with the wchar > > support part, it was added by Nokia. > Ok, so this means that this preference is actually the "wide character > encoding" preference. > To summarize, I'd then suggest to make the following changes: > - Have both CDI and DSF use the same Debug preference page for determining the > default values when opening a new view. In DSF, the popup menu overrides the > preference page. We have been providing our dsf-based debugging views to our customers although we are using our internal debugger (not a gdb). Thus far, we have coded our views to behave what is suggested above. This is what we learn: The 'Default xxx format' in preference page has choices of Natural, Hexadecimal, Decimal and Binary. On the other side, dsf-based views pull down menu Number Format menu has Default, Hex, Decimal, Octal, Binary String. The differences are not only naming convention but some items are missing/extra. Depending on whether you will also add/modify those choices in the preference page, you need to ensure that both CDI and DSF views handle whichever the choice the user enter through the preference page. > - Rename the existing "character encoding" preference to "wide character > encoding", since it's actually used to decode wchar_t strings. > - Add a new "character encoding" preference which is used for decoding ordinary > char strings. This is only used by DSF, and ignored by CDI. > - Add some sort of default option to both of these encoding preferences. I'd > suggest to reuse the same GUI as for example on the General -> Workspace > preferences page, similar to what can be seen in GUI draft 1. > - Add both the "character encoding" as well as the "wide character encoding" > preferences to a submenu of the popup menu of the Variables and Expressions > views in DSF, similar to what can be seen in GUI draft 2. > Mikhail, Marc, are you OK with all of these changes? I understand your customers need this functionality, however, it does not apply to every customer/debugger/compiler. I hope you allow us (dsf view providers) to control whether DSF views to show its String Format menu (GUI draft 2) or not, through an API way.
(In reply to comment #22) > Looks good to me. Does it fall under the "Opened view default settings" or > should we create another section in the preference page? Well yes, this actually could of course also go into a separate "Default character encoding" section. Should we do that? On the other side, it's also an "opened view default setting" as well. > I wonder why have a default entry? Why not just add the string "(Default)" > next to the default choice, and only have one drop-down box to make our > selection? Are we just going for consistency with General -> Workspace? (which > could be a good enough reason, although it wastes space). One reason for me to suggest it that way was indeed consistency with the General -> Workspace preference page. But another reason is that gdb supports a huge number of different charsets. We wouldn't want to put them all into a single dropdown. The General -> Workspace preference page handles this by allowing the user to enter the name of a charset directly into the dropdown as well if it's not contained within the default dropdown list so far. It however may isn't a very good idea to include the "Default" entry into a writeable dropdown, as this may be confusing for users. But it would of course be possible as well. (In reply to comment #23) > > Mikhail, Marc, are you OK with all of these changes? > Sounds good to me. Thanks for volunteering. Thanks for the feedback. (In reply to comment #24) > The 'Default xxx format' in preference page has choices of Natural, > Hexadecimal, Decimal and Binary. On the other side, dsf-based views pull down > menu Number Format menu has Default, Hex, Decimal, Octal, Binary String. The > differences are not only naming convention but some items are missing/extra. Thanks for the info. Yes, we need to work out the numerous subtle differences between CDI and DSF here. > Depending on whether you will also add/modify those choices in the preference > page, you need to ensure that both CDI and DSF views handle whichever the > choice the user enter through the preference page. Right. > it does not apply > to every customer/debugger/compiler. I hope you allow us (dsf view providers) > to control whether DSF views to show its String Format menu (GUI draft 2) or > not, through an API way. Marc, please address this.
(In reply to comment #25) > (In reply to comment #22) > > Looks good to me. Does it fall under the "Opened view default settings" or > > should we create another section in the preference page? > > Well yes, this actually could of course also go into a separate "Default > character encoding" section. Should we do that? On the other side, it's also an > "opened view default setting" as well. Ok, let's leave it in the same section. > One reason for me to suggest it that way was indeed consistency with the > General -> Workspace preference page. But another reason is that gdb supports a > huge number of different charsets. We wouldn't want to put them all into a > single dropdown. The General -> Workspace preference page handles this by > allowing the user to enter the name of a charset directly into the dropdown as > well if it's not contained within the default dropdown list so far. It however > may isn't a very good idea to include the "Default" entry into a writeable > dropdown, as this may be confusing for users. You convinced me. Since the list of choices is very large, it is good to have a Default radio button. > > it does not apply > > to every customer/debugger/compiler. I hope you allow us (dsf view providers) > > to control whether DSF views to show its String Format menu (GUI draft 2) or > > not, through an API way. Good point. We'll figure out a way to make this easily disabled, once it is implemented.
(In reply to comment #25) > We need to work out the numerous subtle differences > between CDI and DSF Okay, let's address this. The following details aren't clear yet: -> What to do with the "Octal" and "String" number formats which are currently supported by DSF? Including them into the preferences page may be a problem since CDI apparently has no support for them. Winnie, how did you handle this? -> How to handle the large number of available charsets within the popup menu? The preference page can use the already mentioned separate default option plus the writable dropdown. However, for the popup menu we either have to a) list only the most important charsets, b) list all supported charsets, c) make an "Other..." option which opens a separate charset selector dialog box, or d) use the same GUI as on the preferences page (i.e. the radio button + writable dropdown) within the popup menu (if that's possible at all). -> In contrast to the number format options which are handled by gdb on a per-variable basis, the string format options are handled on a global basis by gdb. So this means that other parts of DSF which receive strings from gdb like e.g. the debug text hovering feature are affected by a changed charset as well. -> And maybe a few more things as well which I don't think of yet.
(In reply to comment #27) > -> How to handle the large number of available charsets within the popup menu? > The preference page can use the already mentioned separate default option plus > the writable dropdown. However, for the popup menu we either have to a) list > only the most important charsets, b) list all supported charsets, c) make an > "Other..." option which opens a separate charset selector dialog box, or d) use > the same GUI as on the preferences page (i.e. the radio button + writable > dropdown) within the popup menu (if that's possible at all). I like c), which is what draft 2 shows. > -> In contrast to the number format options which are handled by gdb on a > per-variable basis, the string format options are handled on a global basis by > gdb. So this means that other parts of DSF which receive strings from gdb like > e.g. the debug text hovering feature are affected by a changed charset as well. That should be fine. Any encoded string are meant for the user, and the user should be able to choose how to look at them.
(In reply to comment #27) > (In reply to comment #25) > > We need to work out the numerous subtle differences > > between CDI and DSF > Okay, let's address this. The following details aren't clear yet: > -> What to do with the "Octal" and "String" number formats which are currently > supported by DSF? Including them into the preferences page may be a problem > since CDI apparently has no support for them. Winnie, how did you handle this? From ICDIFormat interface, it has two more choices than it presents in the preferenc page, namely, OCTAL and FLOAT. I have no idea why C++ Debug preference page does not present these two choices. I recalled that it was happening quite a long time ago (eclispe 3.2?) but it is possible that CDI views can actually handle those two formats. Someone needs to try it out. From DSF IFormattedValues intterface, it does not have float format. Logically, our view model provider tries to match each format choice in ICDIFormat to the corresponding choice in DSF format. In particular, when a view model provider is "created", its given IPresentationContext object may or may not have a value for the property IDebugVMConstants.PROP_FORMATTED_VALUE_FORMAT_PREFERENCE. If the property is not defined, the vm provider reads cdi preference page and set it to the IPresentationContext (and there is a reason to do this explicitly). Thus the view effectively uses cdt preference. If the property is defined in the given IPresentationContext object, then the view effectively uses the property value and ignore cdi preference. As you guys have discussed, our view treats the cdi preference as a global default and uses it only when the view has no preference defined yet. BTW, our views also listen to cdi preference format change and update dsf side accordingly. This is just a minor customer requirement and is very subjective whether it is good or bad. Since both DSF and CDI has different set of format choices, it looks to me that the preferece page can only show an intersection of these two sets.
(In reply to comment #28) > I like c), which is what draft 2 shows. Me too, but note that this also means that you can't directly see which charset is currently selected within the popup menu when using any of the "other" charsets. The "other" option would be selected within the popup menu, but you'd have to open the selector dialog box to see which charset is actually currently used. But I guess this is still the best option. > That should be fine. > Any encoded string are meant for the user, and the user should be able to > choose how to look at them. Yes, but note that some side effects apply here. Let's assume you currently have the variables and the expressions view open. Let's further assume the variables view uses charset X and the expressions view uses charset Y. The charset of the debug hover window now depends on which of those two views has been refreshed lastly. If it's the variables view, the debug hover window will use charset X. If it's the expressions view, it will be charset Y. This may be just a minor issue since it probably can be assumed that users most often want to use the same charset within different views, as the string encoding format usually only depends on the underlying program which is currently being debugged. So there usually should be little need to use different charsets within different views at the same time. So I wouldn't care about this right now. There are unfortunately yet more things to consider. We are bound to the charsets which are supported by gdb. Getting this list of supported charsets from gdb should hopefully be no problem as they are returned by the "set charset" gdb command (without any parameter). I'll have a look whether there is a "cleaner" way of obtaining the supported gdb charsets though. We however may have a problem with the existing CDI "charset encoding" option here, since it may already includes a charset which gdb can't handle. If we don't want to break compatibility with CDI, we'd also have to include charsets for the "charset encoding" preference which potentially aren't supported by gdb. If the user then selects such a charset, non-ASCII strings will show up as gibberish within DSF. This problem also applies if a gdb < 7.0 is used, since gdb < 7.0 only supports very few charsets. So we somehow have to merge the charsets supported by gdb with the charsets supported by Java either way. It furthermore may be a good idea to either hide or disable the charset encoding options within the popup menu if a gdb < 7.0 is used, since a gdb >= 7.0 is actually required here.
(In reply to comment #30) > So we somehow have to merge the charsets supported by gdb > with the charsets supported by Java either way. I had a look. Using the list of supported charsets from gdb for the GUI is probably a bad idea due to the following reasons: -> gdb needs to be invoked to obtain the list. I guess this is a problem if we just want to display a list of available charsets within the preferences page. -> The only way to list the charsets supported by gdb apparently is by sending the "set charset" command. The response however is just an error string like "Requires an argument. Valid arguments are auto, ASCII, CP367, IBM367, ...". So we would actually have to parse this error string in order to obtain the charset list from gdb. Very ugly. -> It potentially breaks compatibility with the existing CDI "character encoding" preference, since it already supports all charsets of the JVM, and the JVM will usually support different charsets than gdb does. -> It's harder to reuse the already existing org.eclipse.ui.ide.dialogs.EncodingFieldEditor GUI class for the charset selection, since this editor only supports the JVM charsets by default. So it seems that we have to do it this way instead: -> Allow the user to choose any charset supported by the JVM for the "character encoding" and for the "wide character encoding" preferences. It seems that this can be easily implemented by reusing the already mentioned EncodingFieldEditor class. -> If the user selects a charset which is supported by the JVM, but not by gdb, we will recognize that when trying to set the desired charset within the gdb initialization process. In this case we need to fall back to the default encoding, or maybe even to ASCII. This however will happen very rarely since gdb >= 7.0 supports a huge amount of different charsets. For gdb < 7.0 however, we'd actually have to ignore the character encoding preferences entirely. -> gdb might support charsets which can't be selected within the GUI. But I don't think there is anything we can do about it. Marc, what do you think about this?
(In reply to comment #30) > (In reply to comment #28) > > I like c), which is what draft 2 shows. > > Me too, but note that this also means that you can't directly see which charset > is currently selected within the popup menu when using any of the "other" Thanks for pointing this out. I think this is not nice. We could have the selected option dynamically added the visible list. In fact, maybe it would be even nicer to have the visible list be a combination of the most common choices (if you feel there are such) + a few of the most recently selected choices. This is what the drop-down button for Debug or Run does. > > That should be fine. > > Any encoded string are meant for the user, and the user should be able to > > choose how to look at them. > > Yes, but note that some side effects apply here. Let's assume you currently > have the variables and the expressions view open. Let's further assume the > variables view uses charset X and the expressions view uses charset Y. The > charset of the debug hover window now depends on which of those two views has > been refreshed lastly. If it's the variables view, the debug hover window will > use charset X. If it's the expressions view, it will be charset Y. I had missed this point. The charset, being a global setting for GDB is a problem. As your example, if we assume the variables view uses charset X as selected in the proposed drop-down menu, we will set GDB to charset X, correct? Then, if the user selects charset Y for the expressions view, we will set GDB to charset Y. What happens at this point to the variables view? When we step the program, the variables view will get new information from GDB and it will be using charset Y, no? If I understood correctly, this implies that we cannot (using GDB), have a charset per view, but only a global charset. So, if we have both a preference page and a view menu, they will both affect all views. Personally, I would suggest to stick with the global preference page, which suddenly fits quite well, and not implement the view menu, which would not provide anything more, except maybe easier access. (In reply to comment #31) > (In reply to comment #30) > > So we somehow have to merge the charsets supported by gdb > > with the charsets supported by Java either way. > > > I had a look. Using the list of supported charsets from gdb for the GUI is > probably a bad idea due to the following reasons: Thanks for digging deeper. > [...] > So it seems that we have to do it this way instead: > -> Allow the user to choose any charset supported by the JVM for the "character > encoding" and for the "wide character encoding" preferences. It seems that this > can be easily implemented by reusing the already mentioned EncodingFieldEditor > class. > -> If the user selects a charset which is supported by the JVM, but not by gdb, > we will recognize that when trying to set the desired charset within the gdb > initialization process. In this case we need to fall back to the default > encoding, or maybe even to ASCII. This however will happen very rarely since > gdb >= 7.0 supports a huge amount of different charsets. That sounds more than reasonable to me. > For gdb < 7.0 however, > we'd actually have to ignore the character encoding preferences entirely. No problem. There are many features that only work with newer GDBs, that is just the nature of things. > -> gdb might support charsets which can't be selected within the GUI. But I > don't think there is anything we can do about it. No problem, you are still improving things a lot. We don't have to be perfect :) > Marc, what do you think about this? This sounds good to me. P.S. I still have on my plate to write the code for setting the charset, as I promised in Bug 307311. I'll hoping to get to it soon.
(In reply to comment #32) > We could have the > selected option dynamically added the visible list. In fact, maybe it would be > even nicer to have the visible list be a combination of the most common choices > (if you feel there are such) + a few of the most recently selected choices. Yes, of course. Another option which would be less work to implement :-) would be to simply dynamically change the text of the "Other..." option to something like "Other (currently: <X>)...". > The charset, being a global setting for GDB is a > problem. As your example, if we assume the variables view uses charset X as > selected in the proposed drop-down menu, we will set GDB to charset X, correct? > Then, if the user selects charset Y for the expressions view, we will set GDB > to charset Y. What happens at this point to the variables view? When we step > the program, the variables view will get new information from GDB and it will > be using charset Y, no? Unfortunately yes. This could be only solved if each view is able to always set its own charset before receiving the corresponding data from gdb. However, race conditions might appear if communication between gdb and the different views of CDT isn't mutually exclusive. As far as I understand CDT's usage of gdb/mi, communication to gdb isn't synchronized in any way. If that's correct, we indeed can't use different charsets for different views. > If I understood correctly, this implies that we cannot (using GDB), have a > charset per view, but only a global charset. Probably yes. See above. > So, if we have both a preference > page and a view menu, they will both affect all views. That's the reason why I originally proposed to synchronize the preferences page with the popup menu. > Personally, I would > suggest to stick with the global preference page, which suddenly fits quite > well, and not implement the view menu, which would not provide anything more, > except maybe easier access. No, it would only provide easier access. To be honest, providing easy access for users was my sole intention for proposing the popup menu. I think that a "String format" option right next to the "Number format" option would be a nice enhancement. But I agree that it might be confusing to have a global option within the popup menu of a view. What we could maybe do is to include a "Display format preferences..." option or so within the popup menu which simply opens up the corresponding preferences page, so that users can still access the preferences easily. I really feel like there should be some easy access to this, especially because the preferences on the debug page have been ignored by DSF until now. > P.S. I still have on my plate to write the code for setting the charset, as I > promised in Bug 307311. I'll hoping to get to it soon. No problem about that. For reference, the following gdb commands are needed to handle the fix of Bug 307311 plus the user preference option: "set charset" "set host-charset" "set target-charset" "set target-wide-charset" "set print-sevenbit-strings" For completeness, the gdb documentation of these commands can be found at http://sourceware.org/gdb/onlinedocs/gdb/Character-Sets.html and http://sourceware.org/gdb/onlinedocs/gdb/Print-Settings.html
Typo. It should actually be "set print sevenbit-strings" instead of "set print-sevenbit-strings" (no hyphen between "print" and "sevenbit").
(In reply to comment #29) > From ICDIFormat interface, it has two more choices than it presents in the > preferenc page, namely, OCTAL and FLOAT. > I have no idea why C++ Debug preference page does not present these two > choices. I recalled that it was happening quite a long time ago (eclispe 3.2?) > but it is possible that CDI views can actually handle those two formats. > Someone needs to try it out. Thanks Winnie for pointing that out. I tried it. Although ICDIFormat.OCTAL is actually defined, CDI doesn't handle it. It however seems that this could be added quite easy to CDI by editing the org.eclipse.cdt.debug.internal.core.model.CValue class. Marc, should we address this here so that we can build a more consistent preferences page? Should this be a separate bug? Or do we just don't care at all about this? I don't know what ICDIFormat.FLOAT is about. gdb doesn't support a float format for variables at all - http://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Variable-Objects.html says: > The -var-set-format Command > > [...] > > The syntax for the format-spec is as follows: > > format-spec ==> > {binary | decimal | hexadecimal | octal | natural}
Also, what is the exact purpose of the "String" number format in DSF? Do we still need that or will it be obsoleted by the new string encoding preferences?
(In reply to comment #33) > (In reply to comment #32) > Yes, of course. Another option which would be less work to implement :-) would > be to simply dynamically change the text of the "Other..." option to something > like "Other (currently: <X>)...". That is fine with me. > Unfortunately yes. This could be only solved if each view is able to always set > its own charset before receiving the corresponding data from gdb. However, race > conditions might appear if communication between gdb and the different views of > CDT isn't mutually exclusive. As far as I understand CDT's usage of gdb/mi, > communication to gdb isn't synchronized in any way. If that's correct, we > indeed can't use different charsets for different views. It is mostly correct. In the case of variable objects, some commands must be run 'atomically', for example -var-set-format, must be followed directly with -var-evaluate-expression, or else the new format may be changed by some other -var-set-format. But doing something like that would not be simple for this case, because there is no current way to specify the character encoding when asking DSF for a specific variable value. I personally don't think it is worth the effort. > No, it would only provide easier access. To be honest, providing easy access > for users was my sole intention for proposing the popup menu. I think that a > "String format" option right next to the "Number format" option would be a nice > enhancement. But I agree that it might be confusing to have a global option > within the popup menu of a view. I think view menus are expected to work for that particular view. > What we could maybe do is to include a "Display format preferences..." option > or so within the popup menu which simply opens up the corresponding preferences > page, so that users can still access the preferences easily. I really feel like > there should be some easy access to this, especially because the preferences on > the debug page have been ignored by DSF until now. That is an interesting alternative. I don't have enough UI experience to make that call. Mikhail, do you have an opinion? Winnie, would you have any experience from your users about that? Mathias, a follow-up email to cdt-dev may be the way to go, if Mikhail and/or Winnie don't have a firm opinion. (In reply to comment #35) > (In reply to comment #29) > > Thanks Winnie for pointing that out. I tried it. Although ICDIFormat.OCTAL is > actually defined, CDI doesn't handle it. It however seems that this could be > added quite easy to CDI by editing the > org.eclipse.cdt.debug.internal.core.model.CValue class. > > Marc, should we address this here so that we can build a more consistent > preferences page? Should this be a separate bug? Or do we just don't care at > all about this? If we are to share the preference page, we should aim that it works properly for both CDI and DSF. But a separate bug would be ok. I'm mean, the preference page is totally being ignore by DSF right now, so having an ignored OCTAL value seems not as bad. > I don't know what ICDIFormat.FLOAT is about. gdb doesn't support a float format I don't know either. (In reply to comment #36) > Also, what is the exact purpose of the "String" number format in DSF? Do we > still need that or will it be obsoleted by the new string encoding preferences? From VariableVMNode.java: // - If a STRING value format is supported. Then the value label consists of the active format label followed // by the string format. // - If the STRIGN value format is not supported. Then only show the active value format. The GDB reference // implementation currently does not support the string format, but by default it does append extra // information to the value label itself. The STRING format is a special format supported by some backends. I'm not very clear on what it returns since GDB does not support it. Instead, we use our DETAILS format, which gives more information about complex structures. For example, the summary of the content of an array will be shown in that case, instead of {...}
(In reply to comment #37) > But doing something like that would not be simple for this case, because there > is no current way to specify the character encoding when asking DSF for a > specific variable value. I personally don't think it is worth the effort. I guess so. Let's use the global charset preference instead. > Mathias, a follow-up email to cdt-dev may be the way to go, if Mikhail and/or > Winnie don't have a firm opinion. Agreed. > If we are to share the preference page, we should aim that it works properly > for both CDI and DSF. But a separate bug would be ok. I'm mean, the > preference page is totally being ignore by DSF right now, so having an ignored > OCTAL value seems not as bad. I'm going to address this as well, since I don't expect it to be very complicated to add support for the octal number format to CDI. I think I'll then simply rename this bug to "Improving the debug preferences - add support for different charsets and unify DSF and CDI debug preferences" so that this bug has a meaningful title. > > I don't know what ICDIFormat.FLOAT is about. gdb doesn't support a float format > > I don't know either. Since neither DSF nor CDI supports it anyway, I'd say we just don't care about it. > The STRING format is a special format supported by some backends. Ok, so this is the only preference which needs special treatment since it can't be supported by CDI. We can either a) simply exclude it from the preferences page, or b) make it available on the preferences page and fall back to the default value for CDI when it's selected. I'd personally tend to b). Also, CDI calles the default number format "Natural" while DSF calles it "Default". We can only use one of those two. I'd go with "Default" since it's clear that "Default" is simply just a sensible default value. What exactly a "natural" number format is may not be so clear at first sight.
(In reply to comment #37) > I think view menus are expected to work for that particular view. > > What we could maybe do is to include a "Display format preferences..." option > > or so within the popup menu which simply opens up the corresponding preferences > > page, so that users can still access the preferences easily. I really feel like > > there should be some easy access to this, especially because the preferences on > > the debug page have been ignored by DSF until now. > That is an interesting alternative. I don't have enough UI experience to make > that call. Mikhail, do you have an opinion? > Winnie, would you have any experience from your users about that? > Mathias, a follow-up email to cdt-dev may be the way to go, if Mikhail and/or > Winnie don't have a firm opinion. We tried to do so in the past. The feedback is quite negative. Since the user knows Number Format menu in pull down menu is for controlling the entire view's preference, and pop up Number Format menu in is for controlling the individual row format, having a menu item "Display format preferences..." in popup or pull down looks redundant. In addition, when users use the "Display format preferences..." menu item to edit the preference in the Preference page (Draft 1), different users have different expectations. Some says, it should not affect the current view, some say, it should affect the current view. This expectation is very subjective and cannot get everyone happy. Our users find having such a menu item confuse them.
(In reply to comment #38) > > If we are to share the preference page, we should aim that it works properly > > for both CDI and DSF. But a separate bug would be ok. I'm mean, the > > preference page is totally being ignore by DSF right now, so having an ignored > > OCTAL value seems not as bad. > > I'm going to address this as well, since I don't expect it to be very > complicated to add support for the octal number format to CDI. I think I'll > then simply rename this bug to "Improving the debug preferences - add support > for different charsets and unify DSF and CDI debug preferences" so that this > bug has a meaningful title. Ok, thanks. > > > I don't know what ICDIFormat.FLOAT is about. gdb doesn't support a float format > > > > I don't know either. > > Since neither DSF nor CDI supports it anyway, I'd say we just don't care about > it. Unless Mikhail knows of this FLOAT thing, that is ok with me. > > The STRING format is a special format supported by some backends. > > Ok, so this is the only preference which needs special treatment since it can't > be supported by CDI. We can either a) simply exclude it from the preferences > page, or b) make it available on the preferences page and fall back to the > default value for CDI when it's selected. I'd personally tend to b). Ok. > Also, CDI calles the default number format "Natural" while DSF calles it > "Default". We can only use one of those two. I'd go with "Default" since it's > clear that "Default" is simply just a sensible default value. What exactly a > "natural" number format is may not be so clear at first sight. It was a conscious decision in DSF to rename it from "Natural" to "Default", because "Natural" means integer in mathematics. See Bug 258994. I'm ok with renaming the CDI one to "default", but it really should be Mikhail giving his ok.
(In reply to comment #39) > having a menu item "Display format preferences..." in popup or pull > down looks redundant. Hmm. Maybe. But it could also simply be named something like "Configure default display format" and it would look less redundant. > In addition, when users use the "Display format preferences..." menu item to > edit the preference in the Preference page (Draft 1), different users have > different expectations. Some says, it should not affect the current view, some > say, it should affect the current view. This expectation is very subjective and > cannot get everyone happy. Right, but this is independent from any menu item within the view. Users can always just go to the preferences and change them. So whether the views are updated instantly or not when the preferences change is a question which has to be answered either way. (In reply to comment #40) > It was a conscious decision in DSF to rename it from "Natural" to "Default", > because "Natural" means integer in mathematics. See Bug 258994. > > I'm ok with renaming the CDI one to "default", but it really should be Mikhail > giving his ok. Sure.
(In reply to comment #40) > (In reply to comment #38) > > > > > I don't know what ICDIFormat.FLOAT is about. gdb doesn't support a float format > > > > > > I don't know either. > > > > Since neither DSF nor CDI supports it anyway, I'd say we just don't care about > > it. > > Unless Mikhail knows of this FLOAT thing, that is ok with me. > I don't remember, it's been a long time. I guess we thought that FLOAT could be useful for some backends. It is safe to ignore. > I'm ok with renaming the CDI one to "default", but it really should be Mikhail > giving his ok. Using "Default" instead of "Natural" is fine. But all labels visible to users have to be replaced.
(In reply to comment #41) > (In reply to comment #39) > > having a menu item "Display format preferences..." in popup or pull > > down looks redundant. > Hmm. Maybe. But it could also simply be named something like "Configure default > display format" and it would look less redundant. We tried quite a few different wordings, "Configure default display format...", "Display format preferences...", "Configure display format...", "Configure display format preferences...", etc. > > In addition, when users use the "Display format preferences..." menu item to > > edit the preference in the Preference page (Draft 1), different users have > > different expectations. Some says, it should not affect the current view, some > > say, it should affect the current view. This expectation is very subjective and > > cannot get everyone happy. > Right, but this is independent from any menu item within the view. Users can > always just go to the preferences and change them. So whether the views are > updated instantly or not when the preferences change is a question which has to > be answered either way. Maybe I did not make my point clear. Let me rephrase it. (1) When the Draft1 page is brought up from main Windows->Preference menu item, some users expect it should affect all already-opened views, some expect it should not affect any already-opened view. (2) When the Draft1 page is brought up from view's local menu item or pop up menu item, some users expect it should affect current view but not other opened views of the same kind and not other opened views of other kinds. E.g. if the current view is an expression view, they expect it affects that particular expression view only, but not other opened expression views and variables views and registers views. Others users have the same two different expectations as if they brought up the page through main Windows->Preference menu item. Even (1) can be adressed, (2) makes the thing more complicated.
(In reply to comment #43) > We tried quite a few different wordings, "Configure default display format...", > "Display format preferences...", "Configure display format...", "Configure > display format preferences...", etc. > > Maybe I did not make my point clear. Let me rephrase it. > (1) When the Draft1 page is brought up from main Windows->Preference menu item, > some users expect it should affect all already-opened views, some expect it > should not affect any already-opened view. > > (2) When the Draft1 page is brought up from view's local menu item or pop up > menu item, some users expect it should affect current view but not other opened > views of the same kind and not other opened views of other kinds. E.g. if the > current view is an expression view, they expect it affects that particular > expression view only, but not other opened expression views and variables views > and registers views. Others users have the same two different expectations as > if they brought up the page through main Windows->Preference menu item. > > Even (1) can be adressed, (2) makes the thing more complicated. Okay, I see the point. So you suggest to have no "easy access" button / option at all within the views, right?
(In reply to comment #44) > (In reply to comment #43) > > We tried quite a few different wordings, "Configure default display format...", > > "Display format preferences...", "Configure display format...", "Configure > > display format preferences...", etc. > > > > Maybe I did not make my point clear. Let me rephrase it. > > (1) When the Draft1 page is brought up from main Windows->Preference menu item, > > some users expect it should affect all already-opened views, some expect it > > should not affect any already-opened view. > > > > (2) When the Draft1 page is brought up from view's local menu item or pop up > > menu item, some users expect it should affect current view but not other opened > > views of the same kind and not other opened views of other kinds. E.g. if the > > current view is an expression view, they expect it affects that particular > > expression view only, but not other opened expression views and variables views > > and registers views. Others users have the same two different expectations as > > if they brought up the page through main Windows->Preference menu item. > > > > Even (1) can be adressed, (2) makes the thing more complicated. > Okay, I see the point. So you suggest to have no "easy access" button / option > at all within the views, right? Right.
(In reply to comment #42) > I don't remember, it's been a long time. I guess we thought that FLOAT could be > useful for some backends. It is safe to ignore. > Using "Default" instead of "Natural" is fine. But all labels visible to users > have to be replaced. Okay, so these two points are clear then. (In reply to comment #35) > Although ICDIFormat.OCTAL is > actually defined, CDI doesn't handle it. It however seems that this could be > added quite easy to CDI by editing the > org.eclipse.cdt.debug.internal.core.model.CValue class. Mikhail, are you OK with this change in CDI as well? (In reply to comment #45) > > So you suggest to have no "easy access" button / option > > at all within the views, right? > > Right. OK, I'm fine with having no easy access option as well, though I'd personally still prefer to have it. But as you said, this also is a subjective decision. The only reasonable way to find out what would be the best thing to do is to listen to the feedback of an adequate amount of users. But if you say that your customers did mainly react sceptical, then I'd say we probably shouldn't have such an option. Marc, Mikhail, what are your opinions about this? And should we ask on cdt-dev anyway about this?
Mikhail, I just noticed that you've reverted my renaming of this bug before. Should we stay with the old name? I just thought that we should somehow mention the DSF / CDI preference synchronization within the title as well, so that it's clear what this bug is about when referenced later.
(In reply to comment #47) > Mikhail, I just noticed that you've reverted my renaming of this bug before. > Should we stay with the old name? I just thought that we should somehow mention > the DSF / CDI preference synchronization within the title as well, so that it's > clear what this bug is about when referenced later. Mathias, sorry I did it unintentionally. Please restore your name.
Okay. Time for a summary. The proposed changes are as follows: -> Have both CDI and DSF use the same Debug preference page for determining the default number format when opening a new view. In DSF, the popup menu overrides the preferences page. -> The character encoding is a global option and therefore not appropriate for the popup menu of a view in DSF. -> The existing CDI "character encoding" preference is renamed to a "wide character encoding" preference, as this is what it's actually about. -> A new "character encoding" preference is added. It is used for decoding ordinary char strings. It is only used by DSF, and ignored by CDI. -> Both the character encoding and the wide character encoding options use the same GUI as on the General -> Workspace preferences page. -> The GUI supports to select any charset supported by the platform's JVM. This is the same behaviour as the existing CDI "character encoding" preference uses, but only with a different GUI. -> The default charset for ordinary char strings is the platform's JVM default charset. The default charset for wide strings is UTF-16 on Windows, and UTF-32 on Mac / Linux. -> In DSF, if the selected charset isn't supported by gdb, which can only be determined when starting the debug session, we fall back to the default charset. -> The default value for the number format will be renamed from "Natural" to "Default". -> The ICDIFormat.FLOAT value is ignored entirely. -> The number format dropdowns will include a new "Octal" value. DSF can directly use this. CDI is extended to support the octal number format as well. -> The number format dropdowns will include a new "String" value. DSF can directly use this. CDI doesn't support it and simply uses "Default" instead if it should be selected. -> There is no "easy access" feature to the preferences page from within a view in DSF. Users have to figure out themselves that the preferences page is now valid for DSF as well. To be discussed? -> When the charset preferences are changed while currently having a debug session open, the charset is changed instantly in DSF. To be discussed? (Note that this doesn't apply to the number formats, since they only affect newly opened views.) I hope I didn't leave anything out. Please raise any issues.
(In reply to comment #32) > P.S. I still have on my plate to write the code for setting the charset, as I > promised in Bug 307311. I'll hoping to get to it soon. Never mind, I've already figured out how to do that. So you'd just have to review then :-)
(In reply to comment #49) > Okay. Time for a summary. The proposed changes are as follows: > -> Have both CDI and DSF use the same Debug preference page for determining the > default number format when opening a new view. In DSF, the popup menu overrides > the preferences page. > -> The character encoding is a global option and therefore not appropriate for > the popup menu of a view in DSF. > -> The existing CDI "character encoding" preference is renamed to a "wide > character encoding" preference, as this is what it's actually about. > -> A new "character encoding" preference is added. It is used for decoding > ordinary char strings. It is only used by DSF, and ignored by CDI. > -> Both the character encoding and the wide character encoding options use the > same GUI as on the General -> Workspace preferences page. > -> The GUI supports to select any charset supported by the platform's JVM. This > is the same behaviour as the existing CDI "character encoding" preference uses, > but only with a different GUI. > -> The default charset for ordinary char strings is the platform's JVM default > charset. The default charset for wide strings is UTF-16 on Windows, and UTF-32 > on Mac / Linux. > -> In DSF, if the selected charset isn't supported by gdb, which can only be > determined when starting the debug session, we fall back to the default > charset. > -> The default value for the number format will be renamed from "Natural" to > "Default". > -> The ICDIFormat.FLOAT value is ignored entirely. > -> The number format dropdowns will include a new "Octal" value. DSF can > directly use this. CDI is extended to support the octal number format as well. > -> The number format dropdowns will include a new "String" value. DSF can > directly use this. CDI doesn't support it and simply uses "Default" instead if > it should be selected. > -> There is no "easy access" feature to the preferences page from within a view > in DSF. Users have to figure out themselves that the preferences page is now > valid for DSF as well. To be discussed? > -> When the charset preferences are changed while currently having a debug > session open, the charset is changed instantly in DSF. To be discussed? (Note > that this doesn't apply to the number formats, since they only affect newly > opened views.) > I hope I didn't leave anything out. Please raise any issues. As you see in comment #37, STRING is a special format - that even DSF does not treat it as a common format. I just trace the implementation side of DSF-GDB, it does not support STRING. Since the preference page is now being designed to be used for both DSF and CDI, let's keep the choices in that page to be a common set that can be applied to both groups. I would ask to keep 'STRING' out from the choices as it is today, rather than adding it to the list as you suggested here.
(In reply to comment #46) > OK, I'm fine with having no easy access option as well, though I'd personally > still prefer to have it. But as you said, this also is a subjective decision. > The only reasonable way to find out what would be the best thing to do is to > listen to the feedback of an adequate amount of users. But if you say that your > customers did mainly react sceptical, then I'd say we probably shouldn't have > such an option. > > Marc, Mikhail, what are your opinions about this? And should we ask on cdt-dev > anyway about this? Since Winnie provided some user feedback, I'd go with that recommendation. Besides, it is less work for right now, and can be improved later. (In reply to comment #50) > > P.S. I still have on my plate to write the code for setting the charset, as I > > promised in Bug 307311. I'll hoping to get to it soon. > > Never mind, I've already figured out how to do that. So you'd just have to > review then :-) Cool! That is an important 'pattern' to know for DSF-GDB.
Created attachment 210923 [details] GUI proposal 1 Okay, I'm ready with a working patch. First, please check the updated GUI proposal. The character encoding options are now separate since there was consensus that they're global preferences instead of view-specific preferences. I've further improved the wording of the labels to be easier to understand. I think we can go with this new page GUI. Any concerns?
Okay, the patch is functional. There are just a few implementation details which need to be addressed: -> The existing code of the CDebugPreferencePage uses the deprecated Preferences class to store and handle the preference values. The re-used EncodingFieldEditor GUI class however uses a PreferenceStore instead. I worked around this issue for now by simply creating a temporary PreferenceStore for the field editors which is filled with the values from the Preferences object. It works, but it also makes the source code of the preference page even messier. Also, if we keep the deprecated Preferences class, we also have to write deprecated code within the gdb launch sequence, the DSF views, etc. Is there a better solution? Can we rewrite the page to use a PreferenceStore instead? -> Adding support for the octal display format to CDI was easy as it could be done analogous to the already existing hexadecimal and binary display formats. I'm however facing a small problem with the IAddress interface. It already contains the toHexAddressString() and toBinaryAddressString() functions. Adding the required toOctalAddressString() however is an API change, as the interface itself is public. The interface however is only implemented by the Addr32 and Addr64 classes, so I simply directly added the toOctalAddressString() to these classes instead and used the instanceof operator within the calling code. We all know this is really bad style, so should there be an API change instead? -> Another tiny API change is required within the ICDebugConstants interface to adapt the default charset. This interface however is tagged as both @noimplement and @noextend as well, so I guess this is not a problem. Right? -> Just to be sure: org.eclipse.cdt.dsf.gdb.service.command.GDBControl is for gdb < 7.0, while GDBControl_7_0 is for gdb >= 7.0, right?
(In reply to comment #54) > -> Adding support for the octal display format to CDI was easy as it could be > done analogous to the already existing hexadecimal and binary display formats. > I'm however facing a small problem with the IAddress interface. It already > contains the toHexAddressString() and toBinaryAddressString() functions. Adding > the required toOctalAddressString() however is an API change, as the interface > itself is public. The interface however is only implemented by the Addr32 and > Addr64 classes, so I simply directly added the toOctalAddressString() to these > classes instead and used the instanceof operator within the calling code. We > all know this is really bad style, so should there be an API change instead? After thinking again about it, it would of course also be possible to simply display addresses in hex in CDI when using the octal number format. However, note that DSF does actually also display addresses in octal. So this wouldn't be a very consistent behaviour.
(In reply to comment #51) > let's keep the choices in that page to be a > common set that can be applied to both groups. I would ask to keep 'STRING' out > from the choices as it is today Okay, I'll leave the "String" option out, unless Marc thinks we really need it.
Created attachment 211023 [details] Proposed fix for bug 370462 Proposed fix for bug 370462 attached. This patch works fine for me. Things which may still need to be addressed are listed in comment 54.
Created attachment 211025 [details] Updated org.eclipse.cdt.doc.user/images/view_debug_prefs.png file Updated org.eclipse.cdt.doc.user/images/view_debug_prefs.png image attached.
(In reply to comment #57) > Created attachment 211023 [details] > Proposed fix for bug 370462 > > Proposed fix for bug 370462 attached. This patch works fine for me. Things > which may still need to be addressed are listed in comment 54. Also note that the patch for bug 307311 must of course be applied as well for this patch to work.
(In reply to comment #51) > As you see in comment #37, STRING is a special format - that even DSF does not > treat it as a common format. I just trace the implementation side of DSF-GDB, > it does not support STRING. Since the preference page is now being designed to > be used for both DSF and CDI, let's keep the choices in that page to be a > common set that can be applied to both groups. I would ask to keep 'STRING' out > from the choices as it is today, rather than adding it to the list as you > suggested here. Actually, in DSF-GDB the STRING field is allows the user to display the "Details" format, which can give more information. This happens kind of by fluke, since the "details" format is the first in the list. I'm not sure if we should leave it out or not. Let's keep this point open until we have stable solution and we can then have a clearer picture of what to do.
(In reply to comment #54) > Okay, the patch is functional. There are just a few implementation details > which need to be addressed: > > -> The existing code of the CDebugPreferencePage uses the deprecated > Preferences class to store and handle the preference values. The re-used > EncodingFieldEditor GUI class however uses a PreferenceStore instead. I worked > around this issue for now by simply creating a temporary PreferenceStore for > the field editors which is filled with the values from the Preferences object. > It works, but it also makes the source code of the preference page even > messier. Also, if we keep the deprecated Preferences class, we also have to > write deprecated code within the gdb launch sequence, the DSF views, etc. Is > there a better solution? Can we rewrite the page to use a PreferenceStore > instead? If you are willing to migrate to the PreferenceStore, I think this is a good idea. > -> Adding support for the octal display format to CDI was easy as it could be > done analogous to the already existing hexadecimal and binary display formats. > I'm however facing a small problem with the IAddress interface. It already > contains the toHexAddressString() and toBinaryAddressString() functions. Adding > the required toOctalAddressString() however is an API change, as the interface > itself is public. The interface however is only implemented by the Addr32 and > Addr64 classes, so I simply directly added the toOctalAddressString() to these > classes instead and used the instanceof operator within the calling code. We > all know this is really bad style, so should there be an API change instead? I'll have a look when I look at the patch. > -> Another tiny API change is required within the ICDebugConstants interface to > adapt the default charset. This interface however is tagged as both > @noimplement and @noextend as well, so I guess this is not a problem. Right? If the API tool does not complain, that is fine. I'll have a look. > -> Just to be sure: org.eclipse.cdt.dsf.gdb.service.command.GDBControl is for > gdb < 7.0, while GDBControl_7_0 is for gdb >= 7.0, right? Right :)
(In reply to comment #53) > Created attachment 210923 [details] > GUI proposal 1 > > Okay, I'm ready with a working patch. > > First, please check the updated GUI proposal. The character encoding options > are now separate since there was consensus that they're global preferences > instead of view-specific preferences. I've further improved the wording of the > labels to be easier to understand. > > I think we can go with this new page GUI. Any concerns? I like it! I just want to see how resizing the preference window affects the boxes. In the screenshot there is too much wasted space, but I believe a re-size should be done properly and remove that space, if the user so wishes. Thanks Mathias!
(In reply to comment #57) > Created attachment 211023 [details] > Proposed fix for bug 370462 > > Proposed fix for bug 370462 attached. This patch works fine for me. Things > which may still need to be addressed are listed in comment 54. Thanks Mathias. The patch is 445 lines of contribution. Anything above 250 must go through a CQ, and the CQ deadline for the Juno release has passed. We can still get the patch ready to be committed, but we probably won't be able to get it in for Juno. Sorry about that.
(In reply to comment #60) > Actually, in DSF-GDB the STRING field is allows the user to display the > "Details" format, which can give more information. Wouldn't actually be "Details" a more meaningful name then? (In reply to comment #61) > If you are willing to migrate to the PreferenceStore, I think this is a good > idea. This certainly would improve the code, since the current implementation of the preference page is now actually quite messy. But isn't the migration to a preference store a problem with backwards compatibility? I mean, the existing codebase can be updated easily, but what about the existing stored preferences? As far as I understand it, they would be lost. This may be just a minor issue since there aren't many preferences on that page. But still. Or should there be some code which copies the content of the Preferences object to the PreferenceStore if it's empty? > If the API tool does not complain, that is fine. I'll have a look. I think it actually did complain about the ICDebugConstants interface though. It's however tagged as both @noimplement and @noextend, so I thought it may be an error of the API tooling feature, not considering the interface tags. The current patch however definitely contains an API change for the IAddress interface (I've moved the code to the interface again instead of having to call the ugly instanceof operator). This API change however can actually only be a concern if there is any third party code which implements this interface as well, since all implementing classes within CDT have of course been updated by this patch. What's the Eclipse policy here? Ugly code in favor of a tiny API change? Doh. (In reply to comment #62) > I just want to see how resizing the preference window affects the boxes. In > the screenshot there is too much wasted space, but I believe a re-size should > be done properly and remove that space, if the user so wishes. Resizing is handled dynamically, the two boxes just take up the entire horizontal space of the dialog box. This is consistent with the General -> Workspace preference page again, it's done the same way there for the "Text file encoding" and "New text file line delimiter" boxes there. I generally tried to write code which is as consistent as possible with existing behaviour and code style. (In reply to comment #63) > The patch is 445 lines of contribution. Anything above 250 must go through a > CQ, and the CQ deadline for the Juno release has passed. > > We can still get the patch ready to be committed, but we probably won't be able > to get it in for Juno. That's a pity, especially because many lines of this patch are "just" for adding support for the octal number format to CDI. Would it be allowed to handle a "Add support for the octal number format to CDI" bug separately, ending up with two separate patches which are below 250 lines each? Quite questionable, I know. :)
One more note. The current patch yet doesn't handle instant changing of the charset while debugging - i.e. it only sets the charset when launching a new debug session. This can (and should?) of course be changed as well, but I was unsure about where to add this code. The hardest thing for me is generally just finding the correct place where new code should go. So I think all you have to tell me is which classes to edit. I'm also unsure about whether the code which sets the default number format for new variables / expressions / registers views in DSF has been added to the right class. I just did it within FormattedValueVMUtil for now, but it seems this isn't actually the correct place.
Created attachment 211097 [details] org.eclipse.cdt.doc.user/images/view_debug_prefs.png Just did some minor look and feel changes to the view_debug_prefs.png image, so that it has the same appearance as the other screenshots within the user documentation.
(In reply to comment #64) > (In reply to comment #60) > > Actually, in DSF-GDB the STRING field is allows the user to display the > > "Details" format, which can give more information. > Wouldn't actually be "Details" a more meaningful name then? > (In reply to comment #61) > > If you are willing to migrate to the PreferenceStore, I think this is a good > > idea. > This certainly would improve the code, since the current implementation of the > preference page is now actually quite messy. But isn't the migration to a > preference store a problem with backwards compatibility? I mean, the existing > codebase can be updated easily, but what about the existing stored preferences? > As far as I understand it, they would be lost. This may be just a minor issue > since there aren't many preferences on that page. But still. > Or should there be some code which copies the content of the Preferences object > to the PreferenceStore if it's empty? > > If the API tool does not complain, that is fine. I'll have a look. > I think it actually did complain about the ICDebugConstants interface though. > It's however tagged as both @noimplement and @noextend, so I thought it may be > an error of the API tooling feature, not considering the interface tags. > The current patch however definitely contains an API change for the IAddress > interface (I've moved the code to the interface again instead of having to call > the ugly instanceof operator). This API change however can actually only be a > concern if there is any third party code which implements this interface as > well, since all implementing classes within CDT have of course been updated by > this patch. What's the Eclipse policy here? Ugly code in favor of a tiny API > change? Doh. > (In reply to comment #62) > > I just want to see how resizing the preference window affects the boxes. In > > the screenshot there is too much wasted space, but I believe a re-size should > > be done properly and remove that space, if the user so wishes. > Resizing is handled dynamically, the two boxes just take up the entire > horizontal space of the dialog box. This is consistent with the General -> > Workspace preference page again, it's done the same way there for the "Text > file encoding" and "New text file line delimiter" boxes there. I generally > tried to write code which is as consistent as possible with existing behaviour > and code style. > (In reply to comment #63) > > The patch is 445 lines of contribution. Anything above 250 must go through a > > CQ, and the CQ deadline for the Juno release has passed. > > > > We can still get the patch ready to be committed, but we probably won't be able > > to get it in for Juno. > That's a pity, especially because many lines of this patch are "just" for > adding support for the octal number format to CDI. Would it be allowed to > handle a "Add support for the octal number format to CDI" bug separately, > ending up with two separate patches which are below 250 lines each? Quite > questionable, I know. :) I agree to split this bug. The original request of this bug 370462 is to support wide and original char encoding, and so there is a need to let the user to put the user setting in the preference page. So far, I see each the following issues, which are added later in this bug record, can be tracked with its own bug. (1) Make CDI and DSF agree on the same number display formats for variables, registers and expressions views, so that both groups can use the same preference page as an initial setting. We still have a slight disagreement about STRING, DETAILS. (2) Make CDI support OCTAL. This is due to (2) above and Mathias has worked on that. Relating to Mathias work on this item, we are one of the third parties implements IAddress. Too bad, it affects us. I can't come up with any better idea. (3) Make DSF registers/variables/expression views respect the display format choices in the preference page setting as described in (2). Mathias has expressed the right concern in comment #65. I already have code doing this requirement and I can contribute a patch. (4) Rework Preferece page to use preference store. Mathias and Marc has expressed their own concerns. (1) and (4) need to be discussed. (2) has a slight issue to be addressed. For (3), I can contribute or Mathias can continue the work. I suggest we filed these four separated bugs and continue there. We can leaeve 370462 to really foucs on wide char and char encoding preference.
(In reply to comment #67) > I agree to split this bug. The original request of this bug 370462 is to > support wide and original char encoding, and so there is a need to let the user > to put the user setting in the preference page. So far, I see each the > following issues, which are added later in this bug record, can be tracked with > its own bug. > > (1) Make CDI and DSF agree on the same number display formats for variables, > registers and expressions views, so that both groups can use the same > preference page as an initial setting. We still have a slight disagreement > about STRING, DETAILS. > > (2) Make CDI support OCTAL. This is due to (2) above and Mathias has worked on > that. > Relating to Mathias work on this item, we are one of the third parties > implements IAddress. Too bad, it affects us. I can't come up with any better > idea. > > (3) Make DSF registers/variables/expression views respect the display format > choices in the preference page setting as described in (2). Mathias has > expressed the right concern in comment #65. I already have code doing this > requirement and I can contribute a patch. > > (4) Rework Preferece page to use preference store. Mathias and Marc has > expressed their own concerns. > > (1) and (4) need to be discussed. (2) has a slight issue to be addressed. For > (3), I can contribute or Mathias can continue the work. > I suggest we filed these four separated bugs and continue there. > We can leaeve 370462 to really foucs on wide char and char encoding preference. Thanks Winnie for this logical division. I'm not sure what is the proper thing to do with respect to Eclipse Intellectual Property rules. I will ping Doug to see if he can provide guidance.
(In reply to comment #68) As you might have seen on the cdt-dev list, no need to split the patch. Isn't Eclipse a nice community! http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg23713.html I'll still need to open a CQ, but they expect that they will be able to approve it. This means the CQ should be filed ASAP. I'll start reviewing the patch today.
(In reply to comment #69) > As you might have seen on the cdt-dev list, no need to split the patch. Isn't > Eclipse a nice community! Yes indeed :) Marc, thank you very much for taking care about this. Much appreciated! (In reply to msg #23713 on the mailing list) > make sure [...] that there's nothing funky-looking in there So I hope the code isn't actually too funky at all :D > This means the CQ should be filed ASAP. > I'll start reviewing the patch today. Okay, thank you. I'll try to act as fast as possible on any feedback from you. (In reply to comment #67) > Relating to Mathias work on this item, we are one of the third parties > implements IAddress. Thanks for letting us know. I think Marc can shed some light on how to handle this potential API change.
Just an update. I've reviewed the DSF launch sequence changes. Next part to look at is the preferences. Because we are rushed for time, what we are going to do this time, is that I will code the changes myself as a commit on top of Mathias' change. I'll explain the changes of course to make sure Mathias agrees. Since the changes are minor (Mathias did a great job again), this is not a big deal. One thing you should do Mathias, for the future, is turn on the API tool. It tells you if you are making changes that will break the API. For the next version of CDT, we cannot make breaking changes because we are going from CDT 8.0 to CDT 8.1. I've fixed one of the two breaking changes already. I'll explain once I'm done with the review. Here is how to use the API tool: http://wiki.eclipse.org/CDT/policy#Using_API_Tooling
(In reply to comment #71) > Just an update. I've reviewed the DSF launch sequence changes. Next part to > look at is the preferences. Be warned that this code is a little mess. But that's not really my fault :) It would certainly help to just rewrite the entire page to use a PreferenceStore, but I guess there won't be enough time for this now. > Because we are rushed for time, what we are going to do this time, is that I > will code the changes myself as a commit on top of Mathias' change. I'll > explain the changes of course to make sure Mathias agrees. Since the changes > are minor (Mathias did a great job again), this is not a big deal. Thanks for the update. > One thing you should do Mathias, for the future, is turn on the API tool. It > tells you if you are making changes that will break the API. I actually had it turned on. I just wasn't really sure how I could write a clean patch otherwise. I thought that very small API changes might be OK. Sorry for that, I didn't know.
Created attachment 211245 [details] Update for DSF-GDB (In reply to comment #72) Thanks Mathias, great solution for DSF-GDB. I'm looking at the rest now. This is a first patch to slightly update the solution. Changes are: 1- Move code from GDBControl* files to FinalLaunchSequence*. This avoids an non-backwards-compatible API change. I understand that you imitated the solution used for IGDBControl.setPrintPythonErrors(). But when we implemented that code, we didn't have a way to have multiple versions of FinalLaunchSequence, so we had to fallback to using the IGDBControl service. Since then we implemented a was to version FinalLaunchSequence, which helps us nicely for this bug. 2- Replace System.getProperty("os.name").toLowerCase().startsWith("windows") with Platform.getOS().equals(Platform.OS_WIN32) I find this cleaner. Note that I haven't tested this because I don't have easy access to a Windows machine. It would be nice to confirm it works ok. 3- Replace the deprecated: CDebugCorePlugin.getDefault().getPluginPreferences().getString() with Platform.getPreferencesService().getString() I'll be honest that I don't know the details of these APIs. However the second one is not deprecated and is being used elsewhere in DSF-GDB. 4- chain together the asynchronous calls to queueCommand(). QueueCommand() is a DSF asynchronous method that immediately returns, although the work is not finished. The work will be finished when rm.done() is called. The pattern is to call the next method _inside_ the rm.handle* methods. Calling queueCommand() one after the other will cause the same rm.done() method to be called multiple times, which is not right. This is why you might have seen "java.lang.IllegalStateException: RequestMonitor: Sequence "Configuring GDB", result for executing step #7 = Status OK: unknown code=0 OK null, done() method called more than once" 5- In CommandFactory, I moved the new methods to be ordered alphabetically (probably shouldn't be in this case, to keep the methods grouped, but I wanted to be consistent with the rest of the file) 6- Missing or wrong @since tags. Must be @since 4.1 since that is the version of the plugin. Mathias, can you confirm you are ok with these changes? > > One thing you should do Mathias, for the future, is turn on the API tool. It > > tells you if you are making changes that will break the API. > > I actually had it turned on. I just wasn't really sure how I could write a > clean patch otherwise. I thought that very small API changes might be OK. Sorry > for that, I didn't know. The issue is that adding a method to IGDBControl, which is an interface, requires anyone already implementing that interface, to implement this new method. This will break their compilation.
(In reply to comment #73) > 1- Move code from GDBControl* files to FinalLaunchSequence*. This avoids an > non-backwards-compatible API change. I understand that you imitated the > solution used for IGDBControl.setPrintPythonErrors(). But when we implemented > that code, we didn't have a way to have multiple versions of > FinalLaunchSequence, so we had to fallback to using the IGDBControl service. > Since then we implemented a was to version FinalLaunchSequence, which helps us > nicely for this bug. Nice. > 2- Replace > System.getProperty("os.name").toLowerCase().startsWith("windows") > with > Platform.getOS().equals(Platform.OS_WIN32) > I find this cleaner. Note that I haven't tested this because I don't have easy > access to a Windows machine. It would be nice to confirm it works ok. Confirmed to be working on a Windows XP 32 bit machine. I however can't easily verify that it works on a 64 bit Windows machine as well. But I guess so. > 3- Replace the deprecated: > CDebugCorePlugin.getDefault().getPluginPreferences().getString() > with > Platform.getPreferencesService().getString() > I'll be honest that I don't know the details of these APIs. However the second > one is not deprecated and is being used elsewhere in DSF-GDB. Sounds good. > 4- chain together the asynchronous calls to queueCommand(). QueueCommand() is > a DSF asynchronous method that immediately returns, although the work is not > finished. The work will be finished when rm.done() is called. The pattern is > to call the next method _inside_ the rm.handle* methods. Calling > queueCommand() one after the other will cause the same rm.done() method to be > called multiple times, which is not right. This is why you might have seen > "java.lang.IllegalStateException: RequestMonitor: Sequence "Configuring GDB", > result for executing step #7 = Status OK: unknown code=0 OK null, done() method > called more than once" Right, I didn't dig into the details of queueCommand(). Thanks for taking care of this Marc. > 5- In CommandFactory, I moved the new methods to be ordered alphabetically > (probably shouldn't be in this case, to keep the methods grouped, but I wanted > to be consistent with the rest of the file) Sure. > 6- Missing or wrong @since tags. Must be @since 4.1 since that is the version > of the plugin. Oops. Thanks for fixing. > Mathias, can you confirm you are ok with these changes? Looks good. The only thing I wonder about is, do we need to define the default charsets both within the CDebugCorePreferenceInitializer class and within the FinalLaunchSequence_7_0 class again? Can't we somehow re-use the default charsets within the final launch sequence, to that we only have to define them once? > The issue is that adding a method to IGDBControl, which is an interface, > requires anyone already implementing that interface, to implement this new > method. This will break their compilation. I see. So we actually have the same problem with the IAddress interface as well. See comment 55.
(In reply to comment #73) > 2- Replace > System.getProperty("os.name").toLowerCase().startsWith("windows") > with > Platform.getOS().equals(Platform.OS_WIN32) > I find this cleaner. I just think of another way to handle the default charsets. If there is some easy access to the value of sizeof(wchar_t) on the current platform from within CDT, use this for determining the default wide charset instead. And well, gdb >= 7.0 actually supports appropriate default and default wide charsets. Those are used if we don't explicitly set a charset on our own. So this would be another way for handling default charsets - let gdb do the work. The drawback of doing it this way is that we don't actually know what the default charset is, so we also can't present it to the user within the GUI. We could only provide a "Default" option, but the user couldn't know what the default charset actually is.
Mathias, please add in a comment of this bug: a) You have written 100% of the code you are contributing b) You have the right to contribute the code to Eclipse c) Restate that you are self-employed (Copy/paste what you put in the email to cdt-dev is fine)
I have opened CQ 6264 for this contribution: http://dev.eclipse.org/ipzilla/show_bug.cgi?id=6264 My plan is to commit Mathias' patch as is, and commit my minor updates on top of it. I still have some things to review, but any changes will be done as updates once again.
I hereby confirm that --> I've written 100% of the code of this patch on my own in my spare time due to a personal interest. --> I have the will and right to contribute the code to Eclipse, since I'm the sole author of this code. --> I am not employed by any company, but am self-employed instead. I can gladly prove this with an excerpt from the Austrian Professional Register if that should be necessary.
(In reply to comment #77) > I have opened CQ 6264 for this contribution: > http://dev.eclipse.org/ipzilla/show_bug.cgi?id=6264 > > My plan is to commit Mathias' patch as is, and commit my minor updates on top > of it. > > I still have some things to review, but any changes will be done as updates > once again. Sounds good! Thank you once again Marc. :-)
(In reply to comment #78) > I hereby confirm that Thanks, I've updated the CQ with this info.
I'll try to address all points I hadn't addressed before. Sorry for that. The solution is great and the CQ is opened. I'll post my minor modifications, related to APIs. Thanks Mathias! (In reply to comment #48) > -> Have both CDI and DSF use the same Debug preference page for determining the > default number format when opening a new view. In DSF, the popup menu overrides > the preferences page. This does not work for me. FormattedValueVMUtil.getPreferredFormat() is always called with a property already set. In fact, how can we handle the view menu and the default preferences together? How can the user tell the view menu that it should use the preference instead (i.e., unset the view menu selection)? We could have the view menu 'Default' imply to use the preference setting, but that will make it impossible for a user to actually choose the "default" setting from the view menu ("default" would show decimal normally, hex for addresses, etc) > -> The character encoding is a global option and therefore not appropriate for > the popup menu of a view in DSF. Right. > -> The existing CDI "character encoding" preference is renamed to a "wide > character encoding" preference, as this is what it's actually about. Great. > -> A new "character encoding" preference is added. It is used for decoding > ordinary char strings. It is only used by DSF, and ignored by CDI. Ok. > -> Both the character encoding and the wide character encoding options use the > same GUI as on the General -> Workspace preferences page. Nice! > -> The GUI supports to select any charset supported by the platform's JVM. This > is the same behaviour as the existing CDI "character encoding" preference uses, > but only with a different GUI. Perfect. > -> The default charset for ordinary char strings is the platform's JVM default > charset. The default charset for wide strings is UTF-16 on Windows, and UTF-32 > on Mac / Linux. You're the expert. > -> In DSF, if the selected charset isn't supported by gdb, which can only be > determined when starting the debug session, we fall back to the default > charset. By 'default charset' you mean the GDB default one, right? The way I implemented the solution on top of yours is that if an MI command to set a charset fails, we just ignore it (which would be falling back to GDB's default). > -> The default value for the number format will be renamed from "Natural" to > "Default". I've updated all other references that I could find for CDI to follow that change. > -> The ICDIFormat.FLOAT value is ignored entirely. As agreed. > -> The number format dropdowns will include a new "Octal" value. DSF can > directly use this. CDI is extended to support the octal number format as well. Nice. > -> The number format dropdowns will include a new "String" value. DSF can > directly use this. CDI doesn't support it and simply uses "Default" instead if > it should be selected. As agreed, we are not doing this. > -> There is no "easy access" feature to the preferences page from within a view > in DSF. Users have to figure out themselves that the preferences page is now > valid for DSF as well. To be discussed? Ok for now. You can bring this up again later if it something you really want. > -> When the charset preferences are changed while currently having a debug > session open, the charset is changed instantly in DSF. To be discussed? (Note > that this doesn't apply to the number formats, since they only affect newly > opened views.) As you point out, not done yet. (In reply to comment #54) > -> The existing code of the CDebugPreferencePage uses the deprecated > Preferences class to store and handle the preference values. The re-used > EncodingFieldEditor GUI class however uses a PreferenceStore instead. I worked > around this issue for now by simply creating a temporary PreferenceStore for > the field editors which is filled with the values from the Preferences object. > It works, but it also makes the source code of the preference page even > messier. We'll just live with it. > Also, if we keep the deprecated Preferences class, we also have to > write deprecated code within the gdb launch sequence, the DSF views, etc. Is > there a better solution? Can we rewrite the page to use a PreferenceStore > instead? I have fixed that using: Platform.getPreferencesService().getString() > -> Adding support for the octal display format to CDI was easy as it could be > done analogous to the already existing hexadecimal and binary display formats. > I'm however facing a small problem with the IAddress interface. It already > contains the toHexAddressString() and toBinaryAddressString() functions. Adding > the required toOctalAddressString() however is an API change, as the interface > itself is public. Since this API change is not backwards-compatible, we can't do it for this CDT. One way around that is to create a new: interface IAddress2 extends IAddress{} This will allow people already implementing IAddress to keep compiling. But it is an annoying thing to do as interfaces can keep on multiplying. "handle with care". > The interface however is only implemented by the Addr32 and > Addr64 classes, so I simply directly added the toOctalAddressString() to these > classes instead and used the instanceof operator within the calling code. We > all know this is really bad style, so should there be an API change instead? That change would have been ok. However, it would require extendors to make changes to support this, if they are not using Addr32 or Addr64. This is allowed from an API point-of-view. For now, what I will do is display the address in Hex as you suggest in comment 55. It will make it work out-of-the box for everyone. If you want to improve that, you can submit another fix for that issue later on. Personally, I'd focus on improvements to DSF, to reach a larger audience :). > -> Another tiny API change is required within the ICDebugConstants interface to > adapt the default charset. This interface however is tagged as both > @noimplement and @noextend as well, so I guess this is not a problem. Right? - I believe changing the content of a constant is not allowed because anyone that has already compiled with it will not get the change. Therefore previousCompileICDebugConstants.PREF_CHARSET.equals(newlyCompiled.ICDebugConstants.PREF_CHARSET) will fail. - So, we must use PREF_CHARSET to represent the wide character set, and use a new one for the normal character set. - note that the API tools seems broken to detect and report this change. Try changing the content of DEF_CHARSET instead, and you will get an API error. Bug in the API tool? (In reply to comment #65) > The current patch yet doesn't handle instant changing of the charset while > debugging - i.e. it only sets the charset when launching a new debug session. > This can (and should?) of course be changed as well, but I was unsure about > where to add this code. The hardest thing for me is generally just finding the > correct place where new code should go. So I think all you have to tell me is > which classes to edit. Ok, we'll address this once this patch is done. > I'm also unsure about whether the code which sets the default number format for > new variables / expressions / registers views in DSF has been added to the > right class. I just did it within FormattedValueVMUtil for now, but it seems > this isn't actually the correct place. Ah yes, it does not work for me. I'll think about this one. And finally: - Do you want this feature for the "GDB Hardware debugging" launch configuration type? It uses a different FinalLaunchSequence that will need to be udpated separately. My guess is that debugging hardware (using JTAG) does not need this feature. - I have to check what the preference "Show source files in binaries" does. I assume it is ignored by DSF, so we'll need a plan for that, once the rest is all done
Created attachment 211289 [details] Update to avoid API breakage This second patch is also to be added on top of Mathias' solution: it fixes things to avoid breaking APIs, which we are not allowed to do for CDT 8.1. The patch does the following: 1- Remove IAddress.toOctalAddressString() to avoid breaking API. Similarly remove corresponding changes in Addr32, Addr64 and IExpressions.java 2- removed some unecessary API filters (not related to this feature but giving a warning in ICDebugContants) 3- Rename ICDebugConstants.PREF_WIDE_CHARSET to be named ICDebugConstants.PREF_CHARSET as before, to avoid breaking API. 4- Rename ICDebugConstants.PREF_CHARSET to ICDebugConstants.PREF_NON_WIDE_CHARSET 5- For CDI, use IAddress.toHexAddressString() for OCTAL format. Not great, but good enough in my mind. 6- Rename a couple of user-facing strings from "Natural" to "Default" 7- Uncomment the OCTAL string in CDI's NumberFormatsContribution.java. This makes OCTAL a possible choice from teh context-menu->Format for the expressions, registers and variables view (CDI only). Was this forgotten, or left commented on purpose? Mathias, what do you think?
(In reply to comment #81) > FormattedValueVMUtil.getPreferredFormat() is always > called with a property already set. I had the same problem, but I thought this would just happen because I placed the code incorrectly. From my understanding, the getPreferredFormat() function runs when an already opened view wants to get the currently selected number format. But we'd actually need to edit the code which is executed when opening a *new* view instead. There must be some code which runs initially when creating a new variables / expressions / registers view which sets the number format to "Default" there. This code would have to use the setting from the preference page instead. I just didn't manage to find this piece of code. Winnie, can you help us out here? Do you know where this code should go? > In fact, how can we handle the view menu > and the default preferences together? How can the user tell the view menu that > it should use the preference instead (i.e., unset the view menu selection)? I'd say the user can't unset the view menu selection, but each newly opened view uses the setting from the preference page as initial value instead of simply using "Default". This way it also makes sense to have the label text "Default number format" within the GUI - it's the default for opening a new view, but nothing more. > We could have the view menu 'Default' imply to use the preference setting I don't think this is a good idea. You already mentioned the reasons yourself. > > -> In DSF, if the selected charset isn't supported by gdb, which can only be > > determined when starting the debug session, we fall back to the default > > charset. > > By 'default charset' you mean the GDB default one, right? The way I > implemented the solution on top of yours is that if an MI command to set a > charset fails, we just ignore it (which would be falling back to GDB's > default). Right, the current implementation falls back to the gdb default charset, which can be a different default charset than the CDT one. This is OK as long as we only set the charsets within the launch sequence. If we want to support changing the charset instantly while debugging however, we'd have to fall back to the CDT default charset there. This is because if the "set charset" MI command fails, the previously set charset remains active. So we wouldn't fall back to the gdb default in this case, but to the previous charset instead, which we obviously don't want. > Since the IAddress API change is not backwards-compatible, we can't do it for this CDT. > One way around that is to create a new: > interface IAddress2 extends IAddress{} I didn't expect something like that would be allowed by the Eclipse code style guidelines. > - I believe changing the content of a constant is not allowed because anyone > that has already compiled with it will not get the change. You're certainly right about that. > So, we must use PREF_CHARSET to represent the wide character set, and use a > new one for the normal character set. I wanted to avoid that because it's quite misleading to use PREF_CHARSET for the wide charset. But it seems that we have no other choice anyway. > Bug in the API tool? So it seems. > Do you want this feature for the "GDB Hardware debugging" launch > configuration type? It uses a different FinalLaunchSequence that will need to > be udpated separately. My guess is that debugging hardware (using JTAG) does > not need this feature. To be honest, I don't know. I didn't use hardware debugging until now, so I can't tell whether it would make sense to have the charset preferences available there. > - I have to check what the preference "Show source files in binaries" does. I > assume it is ignored by DSF, so we'll need a plan for that, once the rest is > all done I personally don't think it's a problem if this preference is just simply ignored by DSF. But if it actually can be used by DSF in a sensible way, it would of course make sense to use it there as well. (In reply to comment #82) > 1- Remove IAddress.toOctalAddressString() to avoid breaking API. Similarly > remove corresponding changes in Addr32, Addr64 and IExpressions.java Would it also make sense to just comment out the code instead of removing it, so that it can be easily added to a new CDT release where API changes are allowed? > 7- Uncomment the OCTAL string in CDI's NumberFormatsContribution.java. This > makes OCTAL a possible choice from teh context-menu->Format for the > expressions, registers and variables view (CDI only). Was this forgotten, or > left commented on purpose? This was probably commented out because CDI didn't actually support the octal number format until now, but only declared it as a possible number format to be implemented in the future. > Mathias, what do you think? All good.
(In reply to comment #83) > I wanted to avoid that because it's quite misleading to use PREF_CHARSET for > the wide charset. But it seems that we have no other choice anyway. Well, we could create a new PREF_WIDE_CHARSET constant with the "cDebug.character_set" identifier, and deprecate the PREF_CHARSET constant. This way we would have clear identifiers within the code.
(In reply to comment #83) > (In reply to comment #81) > > FormattedValueVMUtil.getPreferredFormat() is always > > called with a property already set. > I had the same problem, but I thought this would just happen because I placed > the code incorrectly. From my understanding, the getPreferredFormat() function > runs when an already opened view wants to get the currently selected number > format. But we'd actually need to edit the code which is executed when opening > a *new* view instead. There must be some code which runs initially when > creating a new variables / expressions / registers view which sets the number > format to "Default" there. This code would have to use the setting from the > preference page instead. I just didn't manage to find this piece of code. > Winnie, can you help us out here? Do you know where this code should go? > > In fact, how can we handle the view menu > > and the default preferences together? How can the user tell the view menu that > > it should use the preference instead (i.e., unset the view menu selection)? > I'd say the user can't unset the view menu selection, but each newly opened > view uses the setting from the preference page as initial value instead of > simply using "Default". This way it also makes sense to have the label text > "Default number format" within the GUI - it's the default for opening a new > view, but nothing more. > > We could have the view menu 'Default' imply to use the preference setting > I don't think this is a good idea. You already mentioned the reasons yourself. I am making a patch ...
(In reply to comment #85) > I am making a patch ... Please don't post anything larger than 250 lines :)
(In reply to comment #84) > Well, we could create a new PREF_WIDE_CHARSET constant with the > "cDebug.character_set" identifier, and deprecate the PREF_CHARSET constant. > This way we would have clear identifiers within the code. Nice idea! So we'll have: @Deprecated PREF_CHARSET PREF_WIDE_CHARSET PREF_NON_WIDE_CHARSET (Any suggestion for a better name? PREF_STD_CHARSET maybe?) I've updated my patch and will post soon.
Created attachment 211341 [details] Update to avoid API breakage 2 (In reply to comment #83) > > In fact, how can we handle the view menu > > and the default preferences together? How can the user tell the view menu that > > it should use the preference instead (i.e., unset the view menu selection)? > > I'd say the user can't unset the view menu selection, but each newly opened > view uses the setting from the preference page as initial value instead of > simply using "Default". This way it also makes sense to have the label text > "Default number format" within the GUI - it's the default for opening a new > view, but nothing more. That is what CDI was doing, so this is good. > > > -> In DSF, if the selected charset isn't supported by gdb, which can only be > > > determined when starting the debug session, we fall back to the default > > > charset. > > > > By 'default charset' you mean the GDB default one, right? The way I > > implemented the solution on top of yours is that if an MI command to set a > > charset fails, we just ignore it (which would be falling back to GDB's > > default). > > Right, the current implementation falls back to the gdb default charset, which > can be a different default charset than the CDT one. This is OK as long as we > only set the charsets within the launch sequence. Ok. > If we want to support > changing the charset instantly while debugging however, we'd have to fall back > to the CDT default charset there. This is because if the "set charset" MI > command fails, the previously set charset remains active. So we wouldn't fall > back to the gdb default in this case, but to the previous charset instead, > which we obviously don't want. I'm not so sure. If the user chooses an invalid charset, it would be ok to simply ignore that command and keep the previously set charset. Anyway, we can discuss this if you choose to support his feature later on. > > interface IAddress2 extends IAddress{} > > I didn't expect something like that would be allowed by the Eclipse code style > guidelines. Annoying but the only way to keep backwards compatibility. So, we live with it. check out IExpressions, IExpressions2, IExpressions3 :) > To be honest, I don't know. I didn't use hardware debugging until now, so I > can't tell whether it would make sense to have the charset preferences > available there. I was never a user of it, so I don't know either. But my guess is that low level hardware debugging will probably not be using something else than ascii. Let's leave it alone. If someone has a need for it, they'll open a bug. > > 1- Remove IAddress.toOctalAddressString() to avoid breaking API. Similarly > > remove corresponding changes in Addr32, Addr64 and IExpressions.java > > Would it also make sense to just comment out the code instead of removing it, > so that it can be easily added to a new CDT release where API changes are > allowed? Of course you're right. If fact, let's go one step further and leave Addr32 and Addr64 as you had them, with the code available. The API problem was only in IAddress. I've reverted my patch along those lines. > > Mathias, what do you think? > > All good. Thanks. So, what's left? 1- Winnie's patch to make DSF respect the preferences for number format 2- Some clarification on what is "Show source files in binaries" Anything else?
(In reply to comment #87) > Nice idea! So we'll have: > @Deprecated PREF_CHARSET > PREF_WIDE_CHARSET > PREF_NON_WIDE_CHARSET (Any suggestion for a better name? PREF_STD_CHARSET > maybe?) I'd say almost everything is better than PREF_NON_WIDE_CHARSET :-) Maybe PREF_STRING_CHARSET or PREF_CHAR_CHARSET? (In reply to comment #88) > Annoying but the only way to keep backwards compatibility. So, we live with > it. check out IExpressions, IExpressions2, IExpressions3 :) Ahhh, and I wondered what the heck do they mean! > leave Addr32 > and Addr64 as you had them, with the code available. The API problem was only > in IAddress. In fact, all that would have to be commented out is the single toOctalAddressString() line within IAddress and the @Override tags. I just wonder whether anybody will think of commenting these lines in when CDT 9.0 ships. Should we open a bug with release target 9.0 for this so that it won't be forgotten then? > Anything else? Maybe this: (In reply to comment #74) > The only thing I wonder about is, do we need to define the default > charsets both within the CDebugCorePreferenceInitializer class and within the > FinalLaunchSequence_7_0 class again? Can't we somehow re-use the default > charsets within the final launch sequence, to that we only have to define them > once? (In reply to comment #75) > I just think of another way to handle the default charsets. If there is some > easy access to the value of sizeof(wchar_t) on the current platform from within > CDT, use this for determining the default wide charset instead.
(In reply to comment #89) > I'd say almost everything is better than PREF_NON_WIDE_CHARSET :-) Maybe > PREF_STRING_CHARSET or PREF_CHAR_CHARSET? Maybe let's just use something like PREF_DEBUG_CHARSET and PREF_DEBUG_WIDE_CHARSET What do you say?
(In reply to comment #90) > PREF_DEBUG_CHARSET and > PREF_DEBUG_WIDE_CHARSET ...or also PREF_GDB_CHARSET PREF_GDB_WIDE_CHARSET
Created attachment 211354 [details] number format part for this bug This is what I would do about number format along with this bug. Let me know if you want me to adjust any changes of it.
(In reply to comment #91) > PREF_DEBUG_CHARSET and > PREF_DEBUG_WIDE_CHARSET I'll use those. Thanks. > PREF_GDB_CHARSET > PREF_GDB_WIDE_CHARSET Since the file ICDebugConstants is part of org.eclipse.cdt.debug.core, we should not 'point' to gdb directly. Other people may use another backend.
> 2- Some clarification on what is "Show source files in binaries" Good news. The "Show source files in binaries" seems to be independent of debugging! It affects how a binary is expanded in the Project Explorer view. Have a look at the attached screenshot of bug 179202. So, it is not being ignored by DSF. The question is why was it put in the 'Debug' preference page? Seems like it belongs elsewhere. Maybe it also meant to affect the Debug Modules view? That view needs some attention for DSF anyway, so I wouldn't worry about it.
(In reply to comment #92) > Created attachment 211354 [details] > number format part for this bug > > This is what I would do about number format along with this bug. Let me know if > you want me to adjust any changes of it. Thanks Winnie! However, I'm not sure the patch does what I thought we agreed to :) Correct me if I'm wrong but changing the global preference should affect any newly opened view, and not existing views. The patch actually does the reverse, where existing views are changed but new views don't follow. Also, I don't think this change belongs in the DSF plugins, but in the DSF-GDB plugins. I'm hoping that this is still ok for your product. If you could use GdbExpressionVMProvider instead of ExpressionVMProvider, and GdbVariableVMProvider instead of VariableVMProvider. We'll need to create a new GdbRegisterVMProvider too. Let me know if you need me to make some of these changes. Thanks
(In reply to comment #95) > (In reply to comment #92) > > Created attachment 211354 [details] > > number format part for this bug > > > > This is what I would do about number format along with this bug. Let me know if > > you want me to adjust any changes of it. > Thanks Winnie! > However, I'm not sure the patch does what I thought we agreed to :) > Correct me if I'm wrong but changing the global preference should affect any > newly opened view, and not existing views. The patch actually does the > reverse, where existing views are changed but new views don't follow. > Also, I don't think this change belongs in the DSF plugins, but in the DSF-GDB > plugins. I'm hoping that this is still ok for your product. > If you could use GdbExpressionVMProvider instead of ExpressionVMProvider, and > GdbVariableVMProvider instead of VariableVMProvider. We'll need to create a > new GdbRegisterVMProvider too. > Let me know if you need me to make some of these changes. > Thanks I am working on it and send out a patch early tomorrow morning...
(In reply to comment #94) > The "Show source files in binaries" seems to be independent of > debugging! [...] The question is why was it put in the 'Debug' preference > page? Probably because this feature is most valuable when debugging? From the description of bug 182388: > When debugging it is really useful to be able to browse the source files > listed in the various symbol tables and see other information about the > executables used in a session. It however sounds a little like that this preference setting has become obsolete with the fix for bug 182388, because comments number 3 and 4 of bug 179202 say: > Until we get the new executables view (bug 182388) done I'll add a pref > setting to control this. > Added a setting to the C++ Debugger pref page. The setting is checked when > a binary is first expanded, hopefully this should do until we deliver 182388.
(In reply to comment #97) > (In reply to comment #94) > > The "Show source files in binaries" seems to be independent of > > debugging! [...] The question is why was it put in the 'Debug' preference > > page? > > Probably because this feature is most valuable when debugging? From the > description of bug 182388: > > > When debugging it is really useful to be able to browse the source files > > listed in the various symbol tables and see other information about the > > executables used in a session. Ok, that makes sense. > It however sounds a little like that this preference setting has become > obsolete with the fix for bug 182388, because comments number 3 and 4 of bug > 179202 say: > > > Until we get the new executables view (bug 182388) done I'll add a pref > > setting to control this. > > > Added a setting to the C++ Debugger pref page. The setting is checked when > > a binary is first expanded, hopefully this should do until we deliver 182388. Yes, I guess we could look into removing it. We'd have to figure out if the Executables view does offer the functionality we propose to remove. We'd have to consult the cdt-dev list and get some approvals too. Since the preference works properly for both CDI and DSF, let's leave it as is for this bug, but you are welcome to contribute this as a separate effort.
Created attachment 211432 [details] number format @Comment 95. You need a patch in platform as well.
Created attachment 211433 [details] number format @Comment 95. This is the platform patch. You need this and the patch for cdt as well
(In reply to comment #95) > (In reply to comment #92) > > Created attachment 211354 [details] > > number format part for this bug > > > > This is what I would do about number format along with this bug. Let me know if > > you want me to adjust any changes of it. > Thanks Winnie! > However, I'm not sure the patch does what I thought we agreed to :) > Correct me if I'm wrong but changing the global preference should affect any > newly opened view, and not existing views. The patch actually does the > reverse, where existing views are changed but new views don't follow. > Also, I don't think this change belongs in the DSF plugins, but in the DSF-GDB > plugins. I'm hoping that this is still ok for your product. > If you could use GdbExpressionVMProvider instead of ExpressionVMProvider, and > GdbVariableVMProvider instead of VariableVMProvider. We'll need to create a > new GdbRegisterVMProvider too. > Let me know if you need me to make some of these changes. > Thanks Marc, I just submit another patch. You need both "number format @Comment 95. You need a patch in platform as well" and "number format @Comment 95. This is the platform patch. You need this and the patch for cdt as well". I do not know how to create a patch from two repos at the same time and so this is why I have to submit two patch files. You do not need "number format part for this bug".
(In reply to comment #101) Thanks Winnie. > I just submit another patch. You need both "number format @Comment 95. You need > a patch in platform as well" and "number format @Comment 95. This is the > platform patch. You need this and the patch for cdt as well". What is the platform patch for? > I do not know how to create a patch from two repos at the same time and so this > is why I have to submit two patch files. I don't think you can. Besides, we'll need to submit them separately anyway. Comments: 1- I think you need to remove && context.getProperty(IDebugVMConstants.PROP_FORMATTED_VALUE_FORMAT_PREFERENCE) == null from all three Gdb*VMProvider.initializeFormat(). When opening a view, we don't want to use the format that is in the presentation context, but we want to use the preference, right? 2- Can we remove FormattedValueVMUtil.translateCdifmt(Object), it is not used. 3- Can we move FormattedValueVMUtil.translateCdifmt(int) out of DSF and into DSF-GDB? Again, I prefer not to have links to CDI. Just create a class org.eclipse.cdt.dsf.gdb.internal.ui.GdbFormattedValueVMUtil. The rest looks good! Thanks.
(In reply to comment #102) > (In reply to comment #101) > Thanks Winnie. > > I just submit another patch. You need both "number format @Comment 95. You need > > a patch in platform as well" and "number format @Comment 95. This is the > > platform patch. You need this and the patch for cdt as well". > What is the platform patch for? This is to make multiple instances of a view to work. > > I do not know how to create a patch from two repos at the same time and so this > > is why I have to submit two patch files. > I don't think you can. Besides, we'll need to submit them separately anyway. > Comments: > 1- I think you need to remove > && > context.getProperty(IDebugVMConstants.PROP_FORMATTED_VALUE_FORMAT_PREFERENCE) > == null > from all three Gdb*VMProvider.initializeFormat(). When opening a view, we > don't want to use the format that is in the presentation context, but we want > to use the preference, right? I thought local preference is expected to be restored from workspace - at least this is the impression I get since 3.6 when we started using dsf - but this may never be requred in dsf-gdb scope. If local preference is needed to be restored from workspace, the platform patch must be there as well. On the other side, if local preference is not needed to be restored from workspace, I can take out this check. This also means I can remove the platform patch. > 2- Can we remove FormattedValueVMUtil.translateCdifmt(Object), it is not used. Yes, it should be removed. > 3- Can we move FormattedValueVMUtil.translateCdifmt(int) out of DSF and into > DSF-GDB? Again, I prefer not to have links to CDI. Just create a class > org.eclipse.cdt.dsf.gdb.internal.ui.GdbFormattedValueVMUtil. I would think FormattedValueVMUtil, which is in org.eclipse.cdt.dsf.ui, is a good place to put helper like this so that other clients can reuse it. Since the dependecies of the involved plugins are not changed, I am not quite following your concern of links to CDI. Maybe you can explain once so that I don't repeat the same thing in the future, while I move the class as you suggested. > The rest looks good! Thanks. In summary, thanks for the feedback and I will update the patch.
Created attachment 211461 [details] number format @Comment 102
(In reply to comment #103) > I thought local preference is expected to be restored from workspace What exactly do you mean by "restore from workspace"? Do you mean that when I close my workspace while having e.g. the variables views open, and I open up my workspace again afterwards, that the number format preference is still the same as before?
> 5- For CDI, use IAddress.toHexAddressString() for OCTAL format. Not great, but > good enough in my mind. I just thought about this again. What do you think of the following instead: [...] else if (CVariableFormat.OCTAL.equals(format)) { // Using the instanceof operator here to avoid API change // Add IAddress.toOctalAddressString() in a future CDT release if (address instanceof Addr32) { return ((Addr32)address).toOctalAddressString(); } else if (address instanceof Addr64) { return ((Addr64)address).toOctalAddressString(); } else { // Fall back to hexadecimal address format return address.toHexAddressString(); } } [...] Also not great, but I think for now it's better. This will display addresses in the correct number format for the vast majority of CDI users, and a future release of CDT can still very easily just comment in the IAddress.toOctalAddressString()
(In reply to comment #106) > > 5- For CDI, use IAddress.toHexAddressString() for OCTAL format. Not great, but > > good enough in my mind. > > I just thought about this again. What do you think of the following instead: > > [...] > else if (CVariableFormat.OCTAL.equals(format)) { > // Using the instanceof operator here to avoid API change > // Add IAddress.toOctalAddressString() in a future CDT release > if (address instanceof Addr32) { > return ((Addr32)address).toOctalAddressString(); > } else if (address instanceof Addr64) { > return ((Addr64)address).toOctalAddressString(); > } else { > // Fall back to hexadecimal address format > return address.toHexAddressString(); > } > } [...] Yes, that is better. I've added a commit with your name for this change. Once the CQ is approved, all this will go in. Thanks for never giving up :)
Created attachment 211478 [details] Fix from Mathias according to comment 106 Here is the patch based on Mathias' suggestion in comment 106. I've made the commit in Mathias' name. Both files already had his name as the last entry for the copyright header, so I didn't add a new identical line. Also, no need to worry about the iplog flag since there will be an iplog+ for one of his patches, which implies the entire bug. Thanks Mathias.
OK, so I think this entire fix should be good to commit then as soon as the last small changes (renaming of the PREF_DEBUG constant etc.) have been made. Marc, do you think the fact that the debugger can now handle different charsets including Unicode is worth to be mentioned at the NewIn81 page on the Wiki? I'd personally like to see it there :-)
(In reply to comment #103) > > What is the platform patch for? > > This is to make multiple instances of a view to work. > > Comments: > > 1- I think you need to remove > > && > > context.getProperty(IDebugVMConstants.PROP_FORMATTED_VALUE_FORMAT_PREFERENCE) > > == null > > from all three Gdb*VMProvider.initializeFormat(). When opening a view, we > > don't want to use the format that is in the presentation context, but we want > > to use the preference, right? Ok, turns out this suggestions makes this worse. With the original code, cloning a view does not use the global preference, which is why I suggested removing it. However, without the code, all views get reset to the preference as soon as we launch a new DSF debug session. > I thought local preference is expected to be restored from workspace Interesting point. But if I understand correctly, this is a bug today. Meaning, that if we restart the workspace, a cloned view will not remember the setting that was chosen in the view menu. If fact, restarting the workspace seems to make all view take the NumberFormat that was chosen last. Please open a bug against the platform and post your patch in that new bug. We can put a dependence from this bug to that new bug if we feel that is necessary. Thanks. > > 3- Can we move FormattedValueVMUtil.translateCdifmt(int) out of DSF and into > > DSF-GDB? Again, I prefer not to have links to CDI. Just create a class > > org.eclipse.cdt.dsf.gdb.internal.ui.GdbFormattedValueVMUtil. > > I would think FormattedValueVMUtil, which is in org.eclipse.cdt.dsf.ui, is a > good place to put helper like this so that other clients can reuse it. Since > the dependecies of the involved plugins are not changed, I am not quite > following your concern of links to CDI. Maybe you can explain once so that I > don't repeat the same thing in the future, while I move the class as you > suggested. You make a good point. We'll have to ask Pawel if he feels this code fits into DSF. I'll ping him for his opinion.
(In reply to comment #109) > OK, so I think this entire fix should be good to commit then as soon as the > last small changes (renaming of the PREF_DEBUG constant etc.) have been made. I've made this change in my local git repo. I guess I forgot to post the patch. This bug is getting confusing :) Git will save the day though :) > Marc, do you think the fact that the debugger can now handle different charsets > including Unicode is worth to be mentioned at the NewIn81 page on the Wiki? I'd > personally like to see it there :-) After all this effort? You better believe it! :) I feel there should be three new entries: 1- the new charset handling 2- DSF's support of the C/C++->Debug preference page. 3- CDI's support of the OCTAL format
(In reply to comment #110) > (In reply to comment #103) > > > What is the platform patch for? > > > > This is to make multiple instances of a view to work. > > > Comments: > > > 1- I think you need to remove > > > && > > > context.getProperty(IDebugVMConstants.PROP_FORMATTED_VALUE_FORMAT_PREFERENCE) > > > == null > > > from all three Gdb*VMProvider.initializeFormat(). When opening a view, we > > > don't want to use the format that is in the presentation context, but we want > > > to use the preference, right? > Ok, turns out this suggestions makes this worse. With the original code, > cloning a view does not use the global preference, which is why I suggested > removing it. > However, without the code, all views get reset to the preference as soon as we > launch a new DSF debug session. Yes I know. So I think we need the check as done in "number format @Comment 95" patch. This means we also need the patch from 37254. > > I thought local preference is expected to be restored from workspace > Interesting point. But if I understand correctly, this is a bug today. > Meaning, that if we restart the workspace, a cloned view will not remember the > setting that was chosen in the view menu. If fact, restarting the workspace > seems to make all view take the NumberFormat that was chosen last. > Please open a bug against the platform and post your patch in that new bug. We > can put a dependence from this bug to that new bug if we feel that is > necessary. Thanks. Here is the bug - https://bugs.eclipse.org/bugs/show_bug.cgi?id=372354 > > > 3- Can we move FormattedValueVMUtil.translateCdifmt(int) out of DSF and into > > > DSF-GDB? Again, I prefer not to have links to CDI. Just create a class > > > org.eclipse.cdt.dsf.gdb.internal.ui.GdbFormattedValueVMUtil. > > > > I would think FormattedValueVMUtil, which is in org.eclipse.cdt.dsf.ui, is a > > good place to put helper like this so that other clients can reuse it. Since > > the dependecies of the involved plugins are not changed, I am not quite > > following your concern of links to CDI. Maybe you can explain once so that I > > don't repeat the same thing in the future, while I move the class as you > > suggested. > You make a good point. We'll have to ask Pawel if he feels this code fits into > DSF. I'll ping him for his opinion. OK. Keep me in the loop.
(In reply to comment #112) > Yes I know. So I think we need the check as done in "number format @Comment 95" > patch. This means we also need the patch from 37254. I'll try your original solution again. Sorry about that. > > > I thought local preference is expected to be restored from workspace > > Interesting point. But if I understand correctly, this is a bug today. > > Meaning, that if we restart the workspace, a cloned view will not remember the > > setting that was chosen in the view menu. If fact, restarting the workspace > > seems to make all view take the NumberFormat that was chosen last. > > Please open a bug against the platform and post your patch in that new bug. We > > can put a dependence from this bug to that new bug if we feel that is > > necessary. Thanks. > > Here is the bug - https://bugs.eclipse.org/bugs/show_bug.cgi?id=372354 Thanks! > > You make a good point. We'll have to ask Pawel if he feels this code fits into > > DSF. I'll ping him for his opinion. > > OK. Keep me in the loop. Ok, so I was wrong :) http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg23778.html Let's use your original proposed patch which puts everything directly in DSF.
(In reply to comment #113) > (In reply to comment #112) > > > Yes I know. So I think we need the check as done in "number format @Comment 95" > > patch. This means we also need the patch from 37254. > > I'll try your original solution again. Sorry about that. I see the problem now. 1- GdbVariableVMProvider (and the others) get created each time a view is opened/cloned; the provider for each instance uses a different presentation context. 2- GdbVariableVMProvider is ALSO created each time a new DSF launch is triggered; in this case, it re-uses the presentation context that the other DSF launches were using for that view instance. The (pseudo) code that we trying to figure out: IPresentationContext context = getPresentationContext(); if (context != null && context.getProperty(PROP_FORMATTED...) == null // line 3 ) { context.setProperty(PROP_FORMATTED..., fmtFromPreferences); } In case 1, when a view is created, the platform fills in PROP_FORMATTED... from the saved memento. Therefore, if we use line 3, we won't re-set the format to the format of the global preferences. This is why I suggested to remove line 3. In case 2 however, when launching a new debug session, we don't want to reset the format of existing views. So we need to keep line 3. What we need is to know is if a view is actually being created or not. The constructor of GdbVariableVMProvider does not tell us that. Is there a class in DSF that gets constructed in that case but not again for every launch? Or is there a way to figure this out from GdbVariableVMProvider? Patrick may have an idea since he did the Cloning feature.
(In reply to comment #114) > What we need is to know is if a view is actually being created or not. The > constructor of GdbVariableVMProvider does not tell us that. Is there a class > in DSF that gets constructed in that case but not again for every launch? Or > is there a way to figure this out from GdbVariableVMProvider? I found one way to do this. We use a new property 'alreadyCreated' that we store in the IPresentationContext. When GdbVariableVMProvider is created, we check the presentation context to see if the property is there. If we find the property, it means this is not a new view, so we don't set the number format; if we don't find the property, we set the number format and we set the property in the context. The trick is that properties get saved in the memento, and therefore that a new context will automatically get the 'alreadyCreated' property from the memento. So, I looked at how the memento is created and I saw that only properties with values of type String, Integer, Boolean or IPersistableElement get stored. Therefore, if we use a value of new Object() for the property, it will not be stored in the memento. I will post a patch for you to try. If you have other suggestions, let me know.
Created attachment 211538 [details] Winnie's patch This is basically Winnie's patch: "number format @Comment 95. You need a patch in platform as well." but I moved all the code back to DSF, since Pawel was ok with it. I will commit it under Winnie's name. Winnie, is that patch ok with you? Next, I will post my update that will use the new 'alreadyCreated' property.
Comment on attachment 211461 [details] number format @Comment 102 I was wrong to ask to move the code to DSF.
(In reply to comment #117) > Comment on attachment 211461 [details] > number format @Comment 102 > I was wrong to ask to move the code to DSF. Marc, I am still digiesting Comments 114 to 118. Maybe you can answer my first question. Is each instance of the same view type required to restore its own view menu setting from restarting a workspace? In particular, do you want (a) last de-activated view wins - it overwrites the settings from all other instances of the same veiw type. This is actually hard for user to predict which instance is the last deacitvated view, because it really depends how eclispe workbench windows behaves when the main window is shutdown. or, (b) each instance has its own setting. Regardless of the order of how views are deactivated, its own setting get saved and restored. It is much more predictable from the angle of persistence. Another related question is, restarting a workspace, do you expect those views in the workspace to behave same as case (1) in Comment#114 or case(2) in Comment#114? I quickly try the idea of 'AlreadyCreated' like the following and it does not check the PROP_FORMATTED with the argument given in Comment#114. protected void initializeFormat() { IPresentationContext context = getPresentationContext(); if (context != null && //context.getProperty(IDebugVMConstants.PROP_FORMATTED_...) == null context.getProperty("AlreadyCreated") == null) { int cdifmt = ... String fmt = ... context.setProperty(IDebugVMConstants.PROP_FORMATTED_..., fmt); context.setProperty("AlreadyCreated", new Object()); } } Is the above code what you are planning? It addresses Comment#114 for both case(1) and (2). However, we lose 'restore setting from workspace'. In fact, my check of 'context.getProperty(IDebugVMConstants.PROP_FORMATTED_...) == null ' is to respect the setting inside a workspace memento so that I do not pull the global preference and I can achieve the purpose of 'restore from workspace'. For case (2), each view instance has its own context instance and that instance's properties is never polluted by other view instances's context properties. On the other side, the pollution happens in case (1) according to VariablesView behavior, even though each view is given its own context object. At the same time I cannot check PROP_FORMATTED; otherwise case(1) will not work. I am running out of ideas...
(In reply to comment #115) > (In reply to comment #114) > > > What we need is to know is if a view is actually being created or not. The > > constructor of GdbVariableVMProvider does not tell us that. Is there a class > > in DSF that gets constructed in that case but not again for every launch? Or > > is there a way to figure this out from GdbVariableVMProvider? As in my comments bug 372354, I think what's more important is to know whether a view is being closed by the user (hence settings should be discarded) or whether they're being closed to shut down workbench, in which case the settings should be preserved. BTW, I'm happy to add whatever is needed in platform, just as long as it makes sense to me :-)
(In reply to comment #119) > (In reply to comment #115) > > (In reply to comment #114) > > > > > What we need is to know is if a view is actually being created or not. The > > > constructor of GdbVariableVMProvider does not tell us that. Is there a class > > > in DSF that gets constructed in that case but not again for every launch? Or > > > is there a way to figure this out from GdbVariableVMProvider? > As in my comments bug 372354, I think what's more important is to know whether > a view is being closed by the user (hence settings should be discarded) or > whether they're being closed to shut down workbench, in which case the settings > should be preserved. This will help solve the workspace restart issue, but it won't help figure out that a brand new cloned view should not use the memento. > BTW, I'm happy to add whatever is needed in platform, just as long as it makes > sense to me :-) Thanks. It's not clear to me yet if the solution should be in platform or CDT, for this specific case. I have to give it some more thought.
(In reply to comment #118) > Marc, I am still digiesting Comments 114 to 118. Maybe you can answer my first > question. Is each instance of the same view type required to restore its own > view menu setting from restarting a workspace? In particular, do you want > (a) last de-activated view wins - it overwrites the settings from all other > instances of the same veiw type. This is actually hard for user to predict > which instance is the last deacitvated view, because it really depends how > eclispe workbench windows behaves when the main window is shutdown. I was thinking to keep things as the platform does now. If that is not good enough, we should fix is separately. But it may not be that simple, as our current improvement may change the current behavior of the restart. > or, > (b) each instance has its own setting. Regardless of the order of how views are > deactivated, its own setting get saved and restored. It is much more > predictable from the angle of persistence. Let's keep this discussion for Bug 372354, as it is complicated in itself. > Another related question is, restarting a workspace, do you expect those views > in the workspace to behave same as case (1) in Comment#114 or case(2) in > Comment#114? It will be case (1) since we don't have an active launch yet. > I quickly try the idea of 'AlreadyCreated' like the following ... > Is the above code what you are planning? Yes, exactly. > It addresses Comment#114 for both case(1) and (2). However, we lose 'restore > setting from workspace'. You are right. I wasn't focusing on the workspace restart because I was trying to get the normal case to work properly. But now that you mention it, my solution actually breaks the workspace restart as even a single view won't recover it's settings. > In fact, my check of > 'context.getProperty(IDebugVMConstants.PROP_FORMATTED_...) == null > ' is to respect the setting inside a workspace memento so that I do not pull > the global preference and I can achieve the purpose of 'restore from > workspace'. I see. But that fails for a normal cloning.... We need something else... > I am running out of ideas... I don't have one either right now. I'll keep thinking about it.
Created attachment 211616 [details] patch depend on 372354 To show the idea how I want to fix, subject to clean-up. I add in-line comment to explain why I change code in that way. You need to pick the patch from Bug 372354. For now, I use GdbVariableVMProvider and so on to show how things I am expecting to work. After changes are agreed to be needed, then we consider to move translateCdifmt in GdbFormattedValueVMUtil to FormattedValueVMUtil, and consider whether initializeFormat in Gdb***VMProvider can be applied to their base class ***VMProvider.
Created attachment 211621 [details] Update for fix of bug 370462.patch I've just discovered a small bug within the GUI of the debug preferences page: when selecting a charset which is equal to the default charset, it won't save it correctly. It instead will jump back to "Default" when opening the preference page again afterwards. I've fixed this within the attached patch. I've further replaced the deprecated getPluginPreferences() function calls within the code of the preferences page with the non-deprecated Platform.getPreferencesService() as Marc suggested earlier. The code of the preference page is now cleaner and doesn't use any deprecated function calls any more. Note that this patch must be applied after all the other patches. I've used the new PREF_DEBUG_CHARSET and PREF_DEBUG_WIDE_CHARSET constants which we've agreed upon, but this patch doesn't include any changes within ICDebugConstants.java, as Marc has said he already made this change within his local repository. I've finally did some quite extensive testing of the new charset feature. I didn't find any further bugs :-)
(In reply to comment #111) > I've made this change in my local git repo. I guess I forgot to post the > patch. This bug is getting confusing :) Yeah... Should we merge all the smaller patches into a single patch which can be applied easily? It's really getting confusing with all these patches, especially because the commit order of the small patches matters. I also think the code which I've initially put into the FormattedValueVMUtil class to initialize the number format is still there. This code isn't needed any more.
(In reply to comment #124) > (In reply to comment #111) > > I've made this change in my local git repo. I guess I forgot to post the > > patch. This bug is getting confusing :) > > Yeah... Should we merge all the smaller patches into a single patch which can > be applied easily? It's really getting confusing with all these patches, > especially because the commit order of the small patches matters. > How about creating a repo on github? I could then push all the commits I have locally so you all can see them. Let me know what you think and if you can create the repo yourself or you'd prefer I do it. > I also think the code which I've initially put into the FormattedValueVMUtil > class to initialize the number format is still there. This code isn't needed > any more. You could fix it yourself in github
(In reply to comment #125) > How about creating a repo on github? > I could then push all the commits I have locally so you all can see them. > Let me know what you think and if you can create the repo yourself or you'd > prefer I do it. I've thought about this and I really should have a github repo so that I can deal with contributions more easily. I'll create one and we'll use it for this bug. I'll post the info.
I've cloned a CDT repo at github that is public. The address is: git@github.com:marckhouzam/cdt.git I've also pushed a branch called: UTFAndPrefs_307311_370462 This branch contains the commit that I have up to now for bug 307311 and this bug (bug 370462). Note that the two last commits (one from Winnie and one from myself) are iffy because we don't have a final solution for the number format synchronization yet. We can build on top of those commits though, and I'll see if I push them all to eclipse.org eventually, or if we reformat them. One can simply add the above address as a new remote in their existing local clone of the CDT repo. Then, just fetch the branch UTFAndPrefs_307311_370462.
(In reply to comment #127) > One can simply add the above address as a new remote in their existing local > clone of the CDT repo. Then, just fetch the branch UTFAndPrefs_307311_370462. Okay, thanks. I'll have a look at it soon, but I unfortunately don't have time right now.
The CQ has been approved! Since we don't have a final solution for the number format sync, I'll have to see what can be committed now and what has to wait. Stay tuned...
(In reply to comment #129) > The CQ has been approved! Glad to hear that! Thanks Marc :) > Since we don't have a final solution for the number > format sync, I'll have to see what can be committed now and what has to wait. I'm sure you're doing things right. Luckily the charset handling is completely independent from the number format handling. They're only placed on the same preference GUI page...
(In reply to comment #127) > UTFAndPrefs_307311_370462 > > This branch contains the commit that I have up to now for bug 307311 and this > bug (bug 370462). Marc, I've finally checked the charset handling part of this patch within the UTFAndPrefs_307311_370462 branch now. Looks good! Only the patch from attachment 211621 [details] is yet missing therein from what I can see. As I currently can hardly find any time for this however, I'm hoping that you can apply this patch as well to the branch and / or the CDT repo as well. Thanks!
I've committed the fixes that were ok for this bug: - "Proposed fix for bug 370462" from Mathias - "Update for DSF-GDB" from me - "Update to avoid API breakage 2" from me - "Fix from Mathias according to comment 106" commit done in Mathias' name What is left: - Number format preference is still not being respected by DSF. This needs bug 372354 to be fixed in platform first. - Fix for preferences that Mathias did in: "Update for fix of bug 370462.patch" This patch, as currently posted does not apply cleanly for me. Mathias, can you update your master branch to the latest and re-post that patch accordingly? Again, thank you Mathias for your hard work and patience.
I also committed the screenshot attached in: "org.eclipse.cdt.doc.user/images/view_debug_prefs.png" in Mathias' name. Note that there is no point in marking "Fix from Mathias according to comment 106" as iplog+ since that attachment is in my name. However, I did do the commit using Mathias' name.
*** cdt git genie on behalf of Marc Khouzam *** Bug 370462: Revert FormattedValueVMUtil to remove previous contribution as it didn't solve the problem. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f4385f2fd69665b60378b46cf50fee3562c90e5b
*** cdt git genie on behalf of Mathias Kunter *** Bug 370462: Improving the debug preferences. Better handling of octal addresses. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=cd1de04157460d7ab7f97b13f83c5d3266114897
*** cdt git genie on behalf of Marc Khouzam *** Bug 370462: Improving the debug preferences - add support for different charsets and unify DSF and CDI debug preferences. Update to number format changes to avoid API breakage. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=49c5be791f23eb89f6db1ac86d8519fec0183f42
*** cdt git genie on behalf of Marc Khouzam *** Bug 370462: Improving the debug preferences - add support for different charsets and unify DSF and CDI debug preferences. Update to DSF-GDB to avoid API change. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=23a8adbdb7b4aa5a80d7d0cbd770efb1022b7b8c
*** cdt git genie on behalf of Mathias Kunter *** Bug 370462: Improving the debug preferences - add support for different charsets and unify DSF and CDI debug preferences [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=c61ae8a137725a03e7b8bb3a71f618aa2fa88b58
*** cdt git genie on behalf of Mathias Kunter *** Bug 370462: Update of screenshot of Debug preference page. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=2cfe871ac8febed3d6de9a2388fe9a07bbb8dab0
(In reply to comment #132) > Mathias, can you update your master branch to the latest and re-post that patch > accordingly? Yup.
Created attachment 212166 [details] Fix according to comment 123 (In reply to comment #132) > Mathias, can you update your master branch to the latest and re-post that patch > accordingly? Done. Hope this patch works now, it's based on current master.
(In reply to comment #141) > Created attachment 212166 [details] > Fix according to comment 123 > > (In reply to comment #132) > > Mathias, can you update your master branch to the latest and re-post that patch > > accordingly? > > Done. Hope this patch works now, it's based on current master. Looks good! Thanks Mathias. Committed to master.
(In reply to comment #142) > Looks good! Thanks Mathias. Committed to master. Thanks Marc. Do we mark this bug as fixed now, or do we wait with that until the DSF number format sync is also done? We could move the number format sync discussion to bug 372354 (or another separate bug), as we're already getting close to 150 comments here on this bug.
*** cdt git genie on behalf of Mathias Kunter *** Bug 370462 - Improving the debug preferences - add support for different charsets and unify DSF and CDI debug preferences [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=baeccceba692449b8bbb77bc851e556820f4fcfd
(In reply to comment #143) (In reply to comment #143) > (In reply to comment #142) > > Looks good! Thanks Mathias. Committed to master. > > Thanks Marc. Do we mark this bug as fixed now, or do we wait with that until > the DSF number format sync is also done? We could move the number format sync > discussion to bug 372354 (or another separate bug), as we're already getting > close to 150 comments here on this bug. Yes, I think that the two issues are distinct. I have opened Bug 373550 to track the number format preference fix. We can (finally :)) close this bug.
