jhc13 / taggui

Tag manager and captioner for image datasets
GNU General Public License v3.0
777 stars 37 forks source link

notify when finished #180

Closed geroldmeisinger closed 5 months ago

geroldmeisinger commented 5 months ago

172

shows a checkbox when captioning multiple images and plays a "tada" sound and show a modal dialog which takes focus when finished

finished

finished

sound is CC0 from https://freesound.org/people/Leszek_Szary/sounds/171670

i also extended the utils.get_confirmation_dialog with a get_confirmation_dialog_checkbox

introduces "playsound" requirement

geroldmeisinger commented 5 months ago

@jhc13: there must be some black magic going on in PySide6 because the checkbox only shows up when I return it as variable, even though it is never accessed directly o_O

def get_confirmation_dialog_checkbox(title: str, question: str, checkbox: str) -> tuple[QMessageBox, QCheckBox]:
    confirmation_dialog = get_confirmation_dialog(title, question)
    checkbox_widget = QCheckBox()
    checkbox_widget.setText(checkbox)
    confirmation_dialog.setCheckBox(checkbox_widget)
    return confirmation_dialog, checkbox_widget

def get_confirmation_dialog_checkbox_reply(title: str, question: str, checkbox: str) -> tuple[int, bool]:
    """Display a confirmation dialog with checkbox and return the user's reply."""
    confirmation_dialog, _ = get_confirmation_dialog_checkbox(title, question, checkbox) # note: checkbox is ignored
    btn = confirmation_dialog.exec()
    checked = confirmation_dialog.checkBox().isChecked()
    return btn, checked

if I change the return type to QMessageBox only, it's gone

@jhc13:

Qsounds didn't work:

        effect = QSoundEffect()
        effect.setSource(QUrl.fromLocalFile("taggui/tada.ogg"))
        effect.play()

or

        player = QMediaPlayer()
        audio_output = QAudioOutput()
        player.setAudioOutput(audio_output)
        player.setSource(QUrl.fromLocalFile("taggui/tada.ogg"))
        player.play()

I get qt.multimedia.ffmpeg.libsymbolsresolver: Couldn't load VAAPI library

geroldmeisinger commented 5 months ago

show error box with error sound with notify on finish. this will only show and play when "[x] Notify on finished". i had to put captioning_thread.run into on big try-catch to catch any error. the exception will be re-raised. error

sound is CC0 https://freesound.org/people/Leszek_Szary/sounds/171497

geroldmeisinger commented 5 months ago

done

jhc13 commented 5 months ago

The playsound3 library changes the log level when it is imported and messes up warning suppression: https://github.com/sjmikler/playsound3/blob/737ed50154f3edee8f0a01d0251b64042bf45490/playsound3/playsound3.py#L18

Qsounds didn't work:

QSoundEffect didn't work for me either with the .ogg files, but it does work with .wav files.

I get qt.multimedia.ffmpeg.libsymbolsresolver: Couldn't load VAAPI library

I got the same warning, but the sound still played, and I was able to remove the warning by installing libva-dev.

jhc13 commented 5 months ago

I wanted to avoid users having to install additional system libraries

I agree, but installing it isn't required in this case (it just removes the warning).

jhc13 commented 5 months ago

I think moving the choice to notify when finished to the settings will be better, because it would allow users to specify more options and have them saved.

As you described in #172, we can let users choose the image count threshold for notification, and also separately decide whether an alert should be shown and whether a sound should be played.

I can implement this myself.

geroldmeisinger commented 5 months ago

I liked it the way it is now because it doesn't introduce additional settings and other bloat (for users and code). I think the only thing missing is that it automatically remembers the last selection. If you clicked "notify" before, default to checked next time, and vice versa. try to use it for a few times. I agree with the sound though because it can be annoying. another idea would be to play the sound when you check "notify when finished"(!), as a reminder and sound check, so users know what's going on. if they don't like it, turn volume down or don't use the feature. no additional options required.

geroldmeisinger commented 5 months ago

I added a setting in https://github.com/jhc13/taggui/pull/185/commits/3afbda6e27eba985d129e606ce9f8f7f9ed4091b just so you know it will conflict (but should be easy to resolve)

jhc13 commented 5 months ago

I liked it the way it is now because it doesn't introduce additional settings and other bloat (for users and code).

I guess it's a matter of personal preference. I don't think adding a few more settings will be problematic. In fact, hiding the checkbox away from the dialog into the settings might actually reduce visual bloat.

another idea would be to play the sound when you check "notify when finished"(!), as a reminder and sound check, so users know what's going on.

Unfortunately, this would mean that a user who does not like the sound would have to suffer through it once, possibly unexpectedly.

geroldmeisinger commented 5 months ago

In fact, hiding the checkbox away from the dialog into the settings might actually reduce visual bloat.

i disagree. it's exactly at the spot were the setting matters. I assume the typical workflow to be:

  1. make a lot of prompts on single images (no message box)
  2. try the good prompt on a few images (don't need notification)
  3. after the prompt passes the test, select all images and make a big run (notification required)

Unfortunately, this would mean that a user who does not like the sound would have to suffer through it once, possibly unexpectedly.

yes. sound off by default and option in the settings.

jhc13 commented 5 months ago

Okay, we can have two checkboxes in the dialog (one for the alert and one for the sound) and save their states to the settings.

geroldmeisinger commented 5 months ago

i meant "[ ] play sound when notify finished" in settings, and only one checkbox in the "start caption" dialog. you either want sounds or not. but you can decide for notifications or not everytime you start a caption.

jhc13 commented 5 months ago

I know, but I don't like the idea of splitting the settings and putting them in two different places.

jhc13 commented 5 months ago

Apparently Windows has default sounds that play when the alert boxes are shown (a different sound for Critical and Information). These sounds currently play at the same time as the custom sounds we added.

I'm thinking maybe we should entirely remove the option to play a sound. If the user is working on the computer when the captioning is finished, then just the alert will be sufficient. If not, and the user is nearby, they can periodically glance at the screen to see if the alert has popped up. And if they are far away, they might not be able to hear the sound, even if it is played.

Another thing to consider is that a large majority of TagGUI users use Windows (stats), and they will be able to let the default sound play if they want it.

geroldmeisinger commented 5 months ago

How about a „execute this script on finish“ setting and have an example script with sound play on success or error. This way users can do anything they want.

On Tuesday 11 June 2024, jhc13 @.***> wrote:

Apparently Windows has default sounds that play when the alert boxes are shown (a different sound for Critical and Information). These sounds currently play at the same time as the custom sounds we added.

I'm thinking maybe we should entirely remove the option to play a sound. If the user is working on the computer when the captioning is finished, then just the alert will be sufficient. If not, and the user is nearby, they can periodically glance at the screen to see if the alert has popped up. And if they are far away, they might not be able to hear the sound, even if it is played.

Another thing to consider is that a large majority of TagGUI users use Windows (stats https://tooomm.github.io/github-release-stats/?username=jhc13&repository=taggui), and they will be able to let the default sound play if they want it.

— Reply to this email directly, view it on GitHub https://github.com/jhc13/taggui/pull/180#issuecomment-2161455566, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2YQWPFYA2B5OMLUJJ5DUQTZG5FALAVCNFSM6AAAAABI74VFU2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRGQ2TKNJWGY . You are receiving this because you authored the thread.Message ID: @.***>

jhc13 commented 5 months ago

I think that's complicating things too much for a feature that very few people will use (most users are probably not programmers).

I'll just remove the sounds and leave the default ones on Windows.