Skip to content
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

[JBIDE 21068] Set maximum length for display name for project wizard #1186

Merged
merged 3 commits into from
May 27, 2016

Conversation

bdshadow
Copy link
Contributor

The limit is, actually, set in swt Text and it is platform dependent. In my Fedora 23

public int getTextLimit () {
    checkWidget ();
    if ((style & SWT.MULTI) != 0) return LIMIT;
    int limit = OS.gtk_entry_get_max_length (handle);
    return limit == 0 ? 0xFFFF : untranslateOffset (limit);
}

always returns 0xFFFF which is exactly 65536. So couldn't test it in a usual way, but just setting the limit lower for testing purposes.
However, i added ValidationStatus.error when input is bigger so that this check works in another OSs.

@jcantrill
Copy link
Contributor

The maximum limit should be limited to what the max allowed by the server.
Looking at the validation code for project it will be the max length
allowed for an annotation. I'm in my mobile so it's a little hard for me to
provide the value

On Wednesday, May 25, 2016, Dmitry notifications@github.com wrote:

The limit is, actually, set in swt Text and it is platform dependent
http://help.eclipse.org/luna/topic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/swt/widgets/Text.html#LIMIT.
In my Fedora 23

public int getTextLimit () {
checkWidget ();
if ((style & SWT.MULTI) != 0) return LIMIT;
int limit = OS.gtk_entry_get_max_length (handle);
return limit == 0 ? 0xFFFF : untranslateOffset (limit);
}

always returns 0xFFFF which is exactly 65536. So couldn't test it in a
usual way, but just setting the limit lower for testing purposes.
However, i added ValidationStatus.error when input is bigger so that this

check works in another OSs.

You can view, comment on, or merge this pull request online at:

#1186
Commit Summary

  • Set maximum length for display name for project wizard

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1186

Jeff Cantrill
Senior Software Engineer, Red Hat Engineering
OpenShift Integration Services
Red Hat, Inc.
Office: 703-748-4420 | 866-546-8970 ext. 8162420
jcantril@redhat.com
http://www.redhat.com

@@ -96,6 +98,9 @@ public IStatus validate(Object value) {
if(param.contains("\t") || param.contains("\n")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would replace "\n" with SWT.LF and "\t" with SWT.TAB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adietish
Copy link
Member

Text.setTextLimit(limit) isnt platform specific. Only the constant Text.LIMIT is platform specific, but we dont need this constant anyhow.
And yes, apparently you need to use the server provided maximum lenght for annotations. @jcantrill can you please show @bdshadow how to retrieve that one?

@bdshadow
Copy link
Contributor Author

@jcantrill , @adietish ok, i'll fix it to the maximum length for annotation.
So maybe we need to edit the task description in jira?

@adietish adietish changed the title Set maximum length for display name for project wizard [JBIDE 21068] Set maximum length for display name for project wizard May 26, 2016
@adietish
Copy link
Member

@bdshadow ok, added an explanation to the jira description for the fact that the maximum length should get derived from the maximum lenght of OpenShift annotations

@adietish
Copy link
Member

@bdshadow can you please always use the JBIDE-XXX identifier in your commit message. This way you can instantly see what jira issue a PR is fixing and the same is ture when reading commits in git log.

@bdshadow
Copy link
Contributor Author

@adietish thank you. Yes, ofcourse, i'll fix it

@fbricon
Copy link
Member

fbricon commented May 26, 2016

Can you please add some unit tests on the validator, you might need to refactor some code to make it testable

@bdshadow
Copy link
Contributor Author

@jcantrill and me discussed it. The only limitation for annotations is that its' total length can't exceed 2^63 (see here). Web Console also doesn't limit the display name. So int64 in go-lang is the maximum value for display name;
The number is really big. Maybe it's a better to leave the limitation in PR as it is?

@fbricon yes, i'll add it


@Override
public IStatus validate(Object value) {
if(value != null && !(value instanceof String))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value != null is redundant with !(value instanceof String)

@fbricon
Copy link
Member

fbricon commented May 26, 2016

Aside from a couple minuscule issues, LGTM. Will merge once @bdshadow updates the PR

@fbricon fbricon merged commit 75914f3 into jbosstools:master May 27, 2016
tsmaeder pushed a commit to tsmaeder/jbosstools-openshift that referenced this pull request Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants