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

Fixed Issue: #4940 Journal entries name can be renamed to blank #655

Merged
merged 1 commit into from Mar 3, 2016

Conversation

devAbnull
Copy link
Contributor

Fixed issue #4940 renaming entries to blank or whitespace. Now entry cannot be renamed to blank (or say whitespace). If tried the old name is retained.

@samdroid-apps
Copy link
Contributor

Looks good! It works well.

However, please change your commit message to the title of the pull request that you used. "Fixed Issue: #4940 Journal entries name can be renamed to blank" tells me a lot about what you did, where as "Fixed Renaming of entry" could refer to anything!

@devAbnull
Copy link
Contributor Author

Done! :)

return
else:
self._model[iterator][ListModel.COLUMN_TITLE] = new_text

self.emit('title-edit-finished')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, I just noticed that if you try and rename it to something empty, this signal will not be emitted. This is an issue, eg. try:

  1. Go into name editing mode in the list view
  2. Rename to "" (empty)
  3. Press enter to confirm
  4. Try typing to search in the journal (don't click in the search entry, that should not be needed), notice that doesn't work though - the entry is not autofocused on keypress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samdroid-apps Do you mean that if there is already existing entry it should be auto-focused on keypress?

Because according to my changes an entry cannot be renamed to empty ("") anyways. But if there is already an entry with empty name then should I make changes to search also such that it auto-focuses on empty named entries.

Is that what you are trying to suggest or there is something more to it?

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to make sure that the "title-edit-finished" is always emitted when the textbox is dismissed. Currently, if the text is set to "" (emtpy) the function returns and the signal is not emitted.

The signal deals with auto-focusing the search bar when you type. To see that feature in action, open the journal and start typing - it will type into the search bar. However, this function is broken if the title-edit-finished is not emitted, see the steps to reproduce that I provided in the previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I just saw the use of the signal 'title-edit-finished' in the journalactivity.py .
I will make the function emit the required signal by adding the line self.emit(..) in if block.
Hope that would be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better don't duplicate code.
Would be better change the 'if' condition to something like:

        if new_text != '' and not new_text.isspace():
             self._model[iterator][ListModel.COLUMN_TITLE] = new_text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, This is a better idea.
@samdroid-apps @godiard I have made the changes. Have a look

@devAbnull
Copy link
Contributor Author

@samdroid-apps I just found that user could save his work with no name (ie "" null)!

Should I fix this?

@devAbnull
Copy link
Contributor Author

To fix this (ie Not allow user to rename or even name his/her work to empty text) we just need to add the condition to check the input text in sugar3/activity/widgets.py save_title() .

@samdroid-apps samdroid-apps merged commit e05452a into sugarlabs:master Mar 3, 2016
@samdroid-apps
Copy link
Contributor

I think that it is harder to deal with it in the activity title entry (as you talked about 3 days ago). This is because here we have a clear feedback mechanism (the name does not change) whereas we don't in the activity title bar. Eg, if we reset it to the default ("%s activity") that would annoy users who were trying to delete the default title and add their own. Maybe we need to think about a design for this one?

@devAbnull
Copy link
Contributor Author

@samdroid-apps Yes I just tried that. Its too annoying.
Couldn't this be solved by just checking for the title only when the user closes the activity ?

@jouravla
Copy link

I found the ticket for this bug (https://bugs.sugarlabs.org/ticket/4940) and found that this issue still persists in the current version of Sugar.

@quozl
Copy link
Contributor

quozl commented Nov 11, 2016

@jouravla, thanks, good point.

Yes, the issue persists in the detail view and activity title, because #655 was not the complete fix, and another fix was not yet merged in #663 before release.

I've updated the ticket.

I've reverted the patch in my OLPC branch.

@quozl
Copy link
Contributor

quozl commented Apr 26, 2017

Revert of #655 submitted as #752.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants