sugarlabs / sugar

Sugar GTK shell
GNU General Public License v3.0
255 stars 241 forks source link

Fixed: Selected entry count in journal could not be updated after erasing a selected entry from the palette. #743

Closed zeeshank95 closed 7 years ago

zeeshank95 commented 7 years ago

This is the link to the bug, https://bugs.sugarlabs.org/ticket/4972#comment:1 When we select multiple files in the journal and if we try to remove a single file by double clicking on the file icon and selecting "erase" option from the palette then the file will be erased but the selected entry count could not get updated. Selecting multiple files and trying to remove a single file from the palette, it does not make any sense and also we have an "erase" option in the toolbar which can be used for erasing multiple files. So when multiple files are selected the palette should not display any "erase" option in the palette, when we double click on the file icon, but it will show the erase option in the palette when no file is selected so that we can remove a single file from the palette only. There will be no issue of the selected entry count, the count will be updated correctly.

quozl commented 7 years ago

While this will fix the problem, it does so in a way that hides a feature; journal entry erase. Yet apart from complexity of fix, there's no reason to hide that feature.

Could you instead deselect an entry before erase?

Also, you have a merge commit in the pull request. Please remove it.

quozl commented 7 years ago

Also, for your interest, your reference to self._journalactivity._editing_mode violates our coding style; as _editing_mode is a private variable; it should be get_editing_mode() instead. By mentioning this I'm not agreeing with the patch, I'm just trying to help you with coding style in future. Reference: PEP 8, Designing for inheritance.

godiard commented 7 years ago

If the journal is in the editing_mode, the individual palettes should not be opened at all.

quozl commented 7 years ago

@godiard wrote:

If the journal is in the editing_mode, the individual palettes should not be opened at all.

Yes, if it were not for our users who have discovered they can use the individual palettes. It is too late for them, and for us to seek the perfect UX now would break what they have discovered.

godiard commented 7 years ago

Using the individual palettes will break in other ways too, by example cloning a item....

quozl commented 7 years ago

Good point, that may have to be handled as well. I've tested, and Duplicate does not cause a problem. Message changed from "Selected 1 of 4" to "Selected 1 of 5". Duplicate entry did not join the selection. Apparently harmless.

A use case is gradual build-up of a selection for a mass Copy or Erase while also resuming, send to project, copying, or erasing individual entries.

zeeshank95 commented 7 years ago

@quozl I can try to deselect an entry before erase. It will solve the problem for 'erase' at least, right?

quozl commented 7 years ago

@zeecoder606, it is easier for me to do it than to explain it. Please review and test #744.

quozl commented 7 years ago

Replaced by #744.