-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
English strings.xml improvements. #1151
English strings.xml improvements. #1151
Conversation
@jamorham Please decide you if these suggestions are in line with your intention. |
Where are the the options, in xDrip, related to lines 42 and 43? |
Thanks for the contribution, but these strings are intended to be generic so I wouldn't propose to merge this. |
@jamorham Which strings are generic? The color related strings does not look like to be generic. The following graph shows my interpretation of the color values which corresponds to stephencmorton string changes. The term "treatment" is generic and could mean either insulin injection or carbohydrate intake. But we have different colors for both of them. Regarding to the barcode strings: I've provided a more detail string the Dexcom G4 Receiver Share barcode, which is indeed a barcode and not a string. But "Show Settings QR code" is indeed a QR code and not a barcode. Hence "Auto configure" scans a QR code as well. Your are right that a QR code is somehow a barcode, namely a matrix or two-dimensional barcode. But it is clearly distinguishable from a barcode. Hence, the proposed string change is more precise and improves the understanding. |
@jamorham Users ask what treatment color or treatment color dark is. If you don't like to merge this, would you please explain how best we can address the question that comes up by the users? |
The graph preview is based on the actual data which sometimes do not contain any customizable color. In the future we should provide a mock graph containing all kind of customizable data. |
@jamorham Please can you explain your concerns or merge this PR? |
"barcode" is changed to "QR code". Clarified meaning of some graph colour settings.
I made this PR two years ago. I thought it would be a nice little un-controversial way to get into this SW. I was just trying to help out, to clarify some obviously incorrect strings. I have 30 years working in embedded SW and I thought I might get into this project. Without in any way doubting the fact that for life-and-death software the utmost care must be taken, and I also admit that I don't understand some of the legal hurdles where perhaps you do not want to name certain things what they absolutely are in actual fact used for, I've got to say that watching the baloney that's gone on with this makes me know that I will never bother contributing to this project again. I'm out. (This is a great tool, and I applaud everybody who works on it. I don't want to leave insulting any of that, kudos to all of you. I just have no desire to work in this environment.) |
<string name="prefs_auto_config_summary">Auto configure using a barcode.</string> | ||
<string name="scan_share2_barcode">Scan Share Barcode</string> | ||
<string name="pref_share2_scan_barcode_summary">Or scan the barcode on the share receiver</string> | ||
<string name="prefs_auto_config_summary">Auto configure using a QR code.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As previously mentioned I think that barcode
is a better word because a QR code is a type of barcode
and so it better represents the feature which can support QR or other barcode types. Changing it to QR code makes it more restrictive regarding what features can be included via the auto-configure scanning and I believe that the word barcode is widely understood to mean all types of barcodes, 1D, 2D etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamorham I noticed that you merged #2022 which changes the icon to a QR code. Given that, I'm ok with keeping the text as "barcode".
At the very least, the juxtaposition clarifies to the users that WE consider the terms interchangeable. 👍
Explicitly calling it a 'barcode' scanner (with the old icon) was making people like myself assume that it explicitly didn't work with QR codes. This was especially confusing because it's only used for QR codes in the current state of XDrip.
<string name="scan_share2_barcode">Scan Share Barcode</string> | ||
<string name="pref_share2_scan_barcode_summary">Or scan the barcode on the share receiver</string> | ||
<string name="prefs_auto_config_summary">Auto configure using a QR code.</string> | ||
<string name="scan_share2_barcode">Scan Share Receiver barcode</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we think this feature is likely obsolete I don't see much point in creating work for translators as I don't think clarification is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we should remove this feature completely from xDrip. This will increase maintainability and also avoids work for translators of new languages like Estonian.
<string name="treatment_color">Insulin On Board Color</string> | ||
<string name="treatment_color_dark">Insulin Activity Color</string> | ||
<string name="predictive_color">Glucose Prediction Color</string> | ||
<string name="predictive_color_dark">COB Prediction Color</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These probably could be changed if that would help users. I think if I'm expanding what appears on the graph in future then I would most likely add additional colors for that purpose. @Navid200 what do you think of these wording changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the word "Color" included at the end redundant as this is a legend and it is only a color legend and nothing else.
So, in documentation, I have the exact same words minus color at the end.
But, below the color legend, on the same page, I have added an image that let's the user also see the patterns for these 4 items. If you scroll down, you will see the graph.
If you look at the insulin activity curve, you can see that it is not easy to see. Only clarifying what the color is may not be enough to help a user what the curve is. Looking at the curve, it's not easy to associate it with the square of color in the legend. That's why I thought it's best to show a typical image.
So, I guess that's a long way of saying I don't love those wording changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but "color" was in the strings before. It was in the strings before it is in the strings after. So you don't object to the change you'd like to make additional changes. You could choose to hold up this PR in order to get everything perfect, I see the benefit in that, although it seems to me that the discussions would likely go on for ever if that was the aim; or you could see if this change at least made things better and didn't make anything worse and realize that you have no objections and will likely create a further PR yourself in the future.
(Ok, this time I'm actually going to unsubscribe from this PR. It just makes me too frustrated.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Navid200 a reminder that incivility is not appropriate here.
In summary:
This PR started as changes suggested by @stephencmorton as to their personal opinion as to how to improve some parts of the user interface. It wasn't to solve any particular raised issue that I know of.
I rejected the graphical changes for aesthetic reasons. @tolot27 redid the QR code graphic with red highlight which I accepted.
I've stated my reasons for not wishing to change the generic term barcode because I feel it constrains what that menu can be used for and so limits future options. I believe the term barcode to be widely understood but if not we can continue to discuss that.
The only remaining parts to this PR is the wording for the predictive simulation colors which were originally left intentionally vague and with a view to future expansion. That time has probably passed and so it is reasonable to consider improving that wording to be better understood by users and I'm simply asking for other's opinion on this to come to a decision. I asked @Navid200 about this as they are likely far closer to understanding how users react to these settings with their experience providing support to users.
This is quite an involved process to change a few words, but I'm simply responding to repeated requests to examine this PR and user interfaces are important to get right. Normally a suggestion to make a change would arise from a specific need. I might be blind to the need to change this text as I of course already know what it means which is why I would ask others for their opinion. I don't think it is a particularly high priority but I'm willing to consider it if others do think its worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use these words:
Insulin on Board
Insulin Activity
Glucose Prediction
Carbs on Board
In addition, many don't know what "Flair Colors" mean.
So, I think it's a good idea to edit those too. But, that could be something else:
#2031
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite an involved process to change a few words, but I'm simply responding to repeated requests to examine this PR and user interfaces are important to get right. Normally a suggestion to make a change would arise from a specific need. I might be blind to the need to change this text as I of course already know what it means which is why I would ask others for their opinion. I don't think it is a particularly high priority but I'm willing to consider it if others do think its worthwhile.
@jamorham a completely understandable reaction and I doubt the OP would have bothered if he'd known it would ultimately explode into such a big discussion. I don't believe it's his (or anyone's) fault that it ended up consuming so much time though.
Are there changes to the process that can be implemented to reduce the overhead to the repo managers? As a new contributor, I've been instructed to keep my PRs as limited in scope/incremental as possible, but that does increase the per-change overhead somewhat. New contributors are also inherently going to start with more "trivial" changes, due to lack of familiarity with the code base.
When it comes to things like translation for example, how do we decide what's "too trivial" to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we proceed with either this PR or #2031 and finally decide which wording is more appropriate?
<string name="scan_share2_barcode">Scan Share Barcode</string> | ||
<string name="pref_share2_scan_barcode_summary">Or scan the barcode on the share receiver</string> | ||
<string name="prefs_auto_config_summary">Auto configure using a QR code.</string> | ||
<string name="scan_share2_barcode">Scan Share Receiver barcode</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we should remove this feature completely from xDrip. This will increase maintainability and also avoids work for translators of new languages like Estonian.
The relevant pieces of this PR were merged via separate PRs so I will close this one. |
These are things that confused me when first using xDrip. I admit I'm new to xDrip so if these changes are not quite right, please let me know and I'll revise them.
English strings.xml improvements.